-
Notifications
You must be signed in to change notification settings - Fork 2
fix: harden amount sanity checks for CoW order and swap flows #225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
twblack88
wants to merge
12
commits into
main
Choose a base branch
from
fix/ss-5630-limit-order-base-units
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a35cbc8
fix: harden CoW order tools against base-unit mistakes
twblack88 0f70271
chore: add local nx cache artifacts
twblack88 363b035
fix: tighten swap and limit price sanity guards
twblack88 9e48a66
fix: run swap balance check before usd hint
twblack88 0db7e43
fix: resolve lint in tool result filtering
twblack88 9750361
chore: add Nx local cache to .gitignore
twblack88 27d1303
fix: harden limit-order validation and LLM output filtering
twblack88 fe6083a
Merge remote-tracking branch 'origin/main' into fix/ss-5630-limit-ord…
0xApotheosis 1f6aacd
fix: filter order outputs in live and replayed model context
0xApotheosis 8d3743c
fix: validate token amounts against precision and available funds
0xApotheosis 87941e0
fix: clarify suspicious limit prices and preserve confirmed targets
0xApotheosis cb3de63
chore: drop redundant Nx ignore addition
0xApotheosis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
apps/agentic-server/src/tools/limitOrder/__tests__/validateLimitPrice.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { describe, expect, test } from 'bun:test' | ||
|
|
||
| import { validateLimitPrice } from '../validateLimitPrice' | ||
|
|
||
| const arb = { symbol: 'ARB', price: '0.5' } | ||
| const usdc = { symbol: 'USDC', price: '1' } | ||
|
|
||
| describe('limit price sanity checks', () => { | ||
| test('asks for clarification on the advertised sub-dollar inversion', () => { | ||
| expect(() => validateLimitPrice('2', arb, usdc)).toThrow('inverted pair price') | ||
| expect(() => validateLimitPrice('2', arb, usdc)).toThrow('confirm the exact target in USDC per 1 ARB') | ||
| expect(() => validateLimitPrice('2', arb, usdc, true)).not.toThrow() | ||
| }) | ||
|
|
||
| test('accepts the market rate and ordinary percentage targets', () => { | ||
| for (const price of ['0.5', '0.525', '0.45']) expect(() => validateLimitPrice(price, arb, usdc)).not.toThrow() | ||
| // ARB $0.12 / EUL $1.39, with a 5% increase in the pair rate. | ||
| expect(() => | ||
| validateLimitPrice('0.09064748201438849', { symbol: 'ARB', price: '0.12' }, { symbol: 'EUL', price: '1.39' }) | ||
| ).not.toThrow() | ||
| }) | ||
|
|
||
| test('flags USD prices used as crypto-to-crypto pair prices', () => { | ||
| const sell = { symbol: 'ARB', price: '0.12' } | ||
| const buy = { symbol: 'EUL', price: '1.39' } | ||
| expect(() => validateLimitPrice('1.39', sell, buy)).toThrow('USD token price') | ||
| expect(() => validateLimitPrice('1.39', sell, buy, true)).not.toThrow() | ||
| }) | ||
|
|
||
| test('allows explicitly confirmed distant future targets', () => { | ||
| expect(() => validateLimitPrice('600', arb, usdc)).toThrow('more than 10×') | ||
| expect(() => validateLimitPrice('600', arb, usdc, true)).not.toThrow() | ||
| expect(() => validateLimitPrice('0.0001', arb, usdc)).toThrow('more than 10×') | ||
| expect(() => validateLimitPrice('0.0001', arb, usdc, true)).not.toThrow() | ||
| }) | ||
|
|
||
| test('does not round tiny pair rates to zero in guidance', () => { | ||
| const sell = { symbol: 'SMALL', price: '0.000001' } | ||
| const buy = { symbol: 'WETH', price: '3000' } | ||
| expect(() => validateLimitPrice('0.000001', sell, buy)).toThrow('3.3333333e-10') | ||
| expect(() => validateLimitPrice('0.000000000333333333', sell, buy)).not.toThrow() | ||
| }) | ||
|
|
||
| test('rejects invalid prices regardless of market data or confirmation', () => { | ||
| for (const value of ['0', '-1', 'NaN', 'Infinity', '1 USDC', '']) { | ||
| expect(() => validateLimitPrice(value, arb, usdc, true)).toThrow('positive number') | ||
| expect(() => validateLimitPrice(value, { symbol: 'ARB', price: '0' }, usdc)).toThrow('positive number') | ||
| } | ||
| }) | ||
|
|
||
| test('does not infer an inversion from unavailable prices', () => { | ||
| for (const price of ['0', 'NaN', 'Infinity', '-1']) { | ||
| expect(() => validateLimitPrice('0.5', { symbol: 'ARB', price }, usdc)).not.toThrow() | ||
| expect(() => validateLimitPrice('0.5', arb, { symbol: 'USDC', price })).not.toThrow() | ||
| } | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
50 changes: 50 additions & 0 deletions
50
apps/agentic-server/src/tools/limitOrder/validateLimitPrice.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import type { Asset } from '@shapeshiftoss/types' | ||
| import BigNumber from 'bignumber.js' | ||
|
|
||
| type PricedAsset = Pick<Asset, 'symbol' | 'price'> | ||
|
|
||
| export function validateLimitPrice( | ||
| value: string, | ||
| sellAsset: PricedAsset, | ||
| buyAsset: PricedAsset, | ||
| priceConfirmed = false | ||
| ): void { | ||
| const price = new BigNumber(value) | ||
| if (!price.isFinite() || !price.gt(0)) { | ||
| throw new Error(`Invalid limitPrice "${value}". It must be a positive number.`) | ||
| } | ||
|
|
||
| const sellUsd = new BigNumber(sellAsset.price ?? '0') | ||
| const buyUsd = new BigNumber(buyAsset.price ?? '0') | ||
| // Missing prices cannot establish an inversion. Basic validation still applies. | ||
| if (!sellUsd.isFinite() || !buyUsd.isFinite() || !sellUsd.gt(0) || !buyUsd.gt(0)) return | ||
|
|
||
| // Cross-multiply comparisons to avoid rounding very small pair prices to zero. | ||
| const targetSellUsd = price.times(buyUsd) | ||
| const materiallyDifferent = targetSellUsd.gte(sellUsd.times(2)) || targetSellUsd.lte(sellUsd.times('0.5')) | ||
| const nearInverse = price.times(sellUsd).minus(buyUsd).abs().lte(buyUsd.times('0.1')) | ||
| const farFromMarket = targetSellUsd.gt(sellUsd.times(10)) || targetSellUsd.lt(sellUsd.times('0.1')) | ||
| const nearUsdPrice = [sellUsd, buyUsd].some(usd => price.minus(usd).abs().lte(usd.times('0.25'))) | ||
|
|
||
| let reason: string | ||
| if (materiallyDifferent && nearInverse) { | ||
| reason = 'looks like an inverted pair price' | ||
| } else if (farFromMarket && nearUsdPrice) { | ||
| reason = 'looks like a USD token price rather than a pair price' | ||
| } else if (farFromMarket) { | ||
| reason = 'differs from the current pair price by more than 10×' | ||
| } else { | ||
| return | ||
| } | ||
|
|
||
| if (priceConfirmed) return | ||
|
|
||
| const Price = BigNumber.clone({ DECIMAL_PLACES: 80 }) | ||
| const marketPrice = new Price(sellUsd).div(buyUsd).toPrecision(8) | ||
| throw new Error( | ||
| `limitPrice ${value} ${buyAsset.symbol}/${sellAsset.symbol} ${reason}. ` + | ||
| `The current market rate is approximately ${marketPrice} ${buyAsset.symbol} per 1 ${sellAsset.symbol}. ` + | ||
| `Ask the user to confirm the exact target in ${buyAsset.symbol} per 1 ${sellAsset.symbol}; do not silently change it. ` + | ||
| `Only after explicit confirmation, retry with that target and priceConfirmed=true.` | ||
| ) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: shapeshift/agentic-chat
Length of output: 32358
LLM Security (CWE-840)
Reachability: External · Exploitability: Moderate
Do not accept
priceConfirmedfrom model-controlled tool input.executeCreateLimitOrderforwards this value tovalidateLimitPrice, wheretruebypasses suspicious-price checks. A schema description cannot enforce prior user confirmation. Derive confirmation from server-side state or bind it to the exact price and asset pair.🤖 Prompt for AI Agents