Audit fix/liquidity allocation #307
Conversation
|
Should we merge this now or try to reduce the bytecode size first? |
contracts/Portfolio.sol
Outdated
| @@ -806,13 +811,13 @@ abstract contract PortfolioVirtual is Objective { | |||
| (, bytes1 instruction) = AssemblyLib.separate(data[0]); // Upper byte is useMax, lower byte is instruction. | |||
|
|
|||
| if (instruction == FVM.ALLOCATE) { | |||
| (uint8 useMax, uint64 poolId, uint128 deltaLiquidity) = | |||
| (uint8 useMax, uint64 poolId, uint128 deltaLiquidity, uint128 deltaAsset, uint128 deltaQuote) = | |||
There was a problem hiding this comment.
personally, I find the deltaAsset and deltaQuote names misleading in this context. It sounds like these are the ones that the protocol will use, but these are the min/max amounts. Could consider something like desiredDeltaAsset, expectedDeltaAsset or just maxDeltaAsset instead.
There was a problem hiding this comment.
That's reasonable, I'll change these names.
| gt(input, 0xffffffffffffffffffffffffffffffff), | ||
| gt(output, 0xffffffffffffffffffffffffffffffff) |
There was a problem hiding this comment.
the overflow checks don't work like this because the overflow can already have happened in output := mul(base1, exp(10, power1)). base1 = 1, power1 = 78 => mul(1, 1e78) overflows 256 bits and would result in the value mod 2**256. Maybe you should just do the decoding in assembly and the final multiplication for input and output in Solidity as uint128 which defaults to multiplication with overflow checks.
There was a problem hiding this comment.
Same issue at the other decoding functions
There was a problem hiding this comment.
Or if you want to stay in assembly you could do the reverse check in addition to checking if it's < 2**128:
p1 := exp(10, power1) // assuming power1 < 78. this can also overflow and should be checked
output := mul(base1, p1)
// something like this: should work iff output did not overflow
eq(base1, div(output, p1))There was a problem hiding this comment.
Thanks for pointing that out, I missed that part. I just pushed a fix for the decodeSwap function, if it's correct I'll update the rest of the decoding functions.
There was a problem hiding this comment.
The updated version looks correct to me. I'll note that the operations to determine length0 and length1 could have underflow as well, but I think the most likely consequence of that is both swap values being zero (base0 and base1 will get right shifted out of existence) so shouldn't be a big risk to a caller that messes up their calldata.
There was a problem hiding this comment.
agree, could add a quick check that length0/length1 <= 256/8=32 so the sub(256, mul(8, length0)) doesn't underflow. this restricts base0/1 to 32 bytes.
| } else if (instruction == FVM.DEALLOCATE) { | ||
| (uint8 useMax, uint64 poolId, uint128 deltaLiquidity) = | ||
| (uint8 useMax, uint64 poolId, uint128 deltaLiquidity, uint128 minDeltaAsset, uint128 minDeltaQuote) = | ||
| FVM.decodeAllocateOrDeallocate(data); | ||
| _deallocate(useMax == 1, poolId, deltaLiquidity); |
There was a problem hiding this comment.
is this a GitHub display bug? For me, it looks like you're calling _deallocate(useMax == 1, poolId, deltaLiquidity); without the min amounts which should be impossible because you changed _deallocate to take the two additional parameters.
There was a problem hiding this comment.
Good catch, looks like it was not a display bug; fixed now: cbce5a7
There was a problem hiding this comment.
I noticed that yesterday while merging but then fixed it on the spearbit/post-audit branch, I just don't know what happened, it looks like resolving the conflicts resulted in a weird merge...
| } | ||
| let decimals := exp(10, power) | ||
|
|
||
| let length := sub(pointer1, 12) |
There was a problem hiding this comment.
this can underflow as well if someone creates a bad encoding that points into the first 11 bytes (useMax, poolid, pointer1, pointer2). could check for underflow here too
I squeezed 2 fixes in this PR as I felt like they were closely related (see https://github.com/spearbit-audits/review-primitive/issues/10 and https://github.com/spearbit-audits/review-primitive/issues/4):
Liquidity can be (de-)allocated at a bad price: I added 2 extra parameters when allocating / deallocating liquidity, so users can now specify the expected amount to pay in asset / quote tokens when providing liquidity and also the expected amount of asset / quote tokens to receive when removing liquidity.Unsafe type-casting: packed amounts in thedecodeAllocateOrDeallocatefunction were vulnerable to overflow. I added an extra check to patch this. And sincedecodeSwapanddecodeCreatePoolwere sharing the same issue and fixed them too.