danielmkm
commented
4 months ago Interesting to check this in scaled18 values. When I heard about the problem I thought of doing it the other way around (scale the minimum amount to raw, and not the opposite).
This solution does not prevent the trade of 1 wei USDC, which is a 6 decimals (1 wei USDC, when converted to scaled18, is 1e12, which is greater than 1e6) so this solution seems effective only for tokens with more than 12 decimals, but not sure if this is an issue.
Trading 1 wei USDC is not inherently dangerous, all pool math is performed on scaled 18 values, so rounding errors will always be limited to the last digits in scaled 18. The goal of this check is to provide another layer of security that helps to ensure that rounding errors in the last digits do not impact the amount calculated in ways that could potentially have down stream impact (ie: pumping the rate). A token with 6 decimals is already "safe" because any sort of rounding errors will be wiped out when we down scale back to raw amounts
EndymionJkb
Description
This is an aggressive form of enforcing a minimum trade amount in the critical path, at the Vault level. Per discussion, it's a fixed value applied to scaled amounts. It checks amountGiven/amountCalculated on swaps, and also non-zero amountsIn/amountsOut on liquidity operations (allowing zeros to support general unbalanced operations).
We had originally wanted to do this at the pool level (like the swap fee limits). The issue there is liquidity operations (and theoretically even the swaps) don't have to go through the pool, so there would be no way to enforce the limits across all pools. (So, for instance, an attacker could just make a pool that didn't have any limits, and potentially use it to exploit core pools.)
In the present implementation, is generally allowing zeros on liquidity operations too permissive? The alternative would be to treat the different kinds differently, which sounds like a lot of bytecode.
One consequence of this approach is we can no longer have a 100% fee (see changes to the hook tests). That would be a UX issue, as the devs would need to know that they can't go to 100% (and the exact upper limit would depend on the pool state). A solution to that would be to do swaps the same as liquidity operations: allow 0, and only enforce the minimum for 1 wei and above. This would "fix" all the hook tests and reduce the diff.
So the main question is whether we can provide sufficient protection against small "attack" trades, without compromising UX. A check that has holes in it isn't a good check. It's actually counterproductive: adding complexity and bytecode without providing the full benefit.
Another approach we'd considered earlier was a "fudge factor" - not trying to limit the trade size, but just adding 1, 10, or 100 wei to all amountsIn, and deducting the same from amountsOut. I'm a bit worried that could actually be exploited itself (e.g., an attacker trying to get 0 out for something in could craft a trade that results in 1 out, knowing that we will subtract one).
Should we have this at all, or a fudge factor instead, or both? The central idea here is simple: we are saying that there is no legitimate reason to trade tiny wei amounts - those are either fat fingered inputs or attacks - and blocking these trades should help prevent many classes of attacks (e.g., the V2 LinearPool exploit). The research question is whether we can do this effectively, fit within the bytecode limit, and not create more problems than we solve.
Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution