Platform: Code4rena
Start Date: 26/09/2022
Pot Size: $50,000 USDC
Total HM: 13
Participants: 113
Period: 5 days
Judge: 0xean
Total Solo HM: 6
Id: 166
League: ETH
Rank: 8/113
Findings: 2
Award: $1,516.94
🌟 Selected for report: 1
🚀 Solo Findings: 0
1464.8726 USDC - $1,464.87
https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L416-L485 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L589-L623 https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L626-L673
Missing slippage control system. Users may lose a lot of funds due to front-running MEV bots. It has liquidityDesired or amountRequired but these parameters are only used in output amount calculation. It isn't used to prevent the output amounts from dropping below a threshold. So, MEV bots can front-run to profit from dropping the output amount of user swap.
function mint( address sender, address recipient, int24 bottomTick, int24 topTick, uint128 liquidityDesired, bytes calldata data ) external override lock onlyValidTicks(bottomTick, topTick) returns ( uint256 amount0, uint256 amount1, uint128 liquidityActual ) { require(liquidityDesired > 0, 'IL'); { (int256 amount0Int, int256 amount1Int, ) = _getAmountsForLiquidity( bottomTick, topTick, int256(liquidityDesired).toInt128(), globalState.tick, globalState.price ); amount0 = uint256(amount0Int); amount1 = uint256(amount1Int); } uint256 receivedAmount0; uint256 receivedAmount1; { if (amount0 > 0) receivedAmount0 = balanceToken0(); if (amount1 > 0) receivedAmount1 = balanceToken1(); IAlgebraMintCallback(msg.sender).algebraMintCallback(amount0, amount1, data); if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM'); if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM'); } liquidityActual = liquidityDesired; if (receivedAmount0 < amount0) { liquidityActual = uint128(FullMath.mulDiv(uint256(liquidityActual), receivedAmount0, amount0)); } if (receivedAmount1 < amount1) { uint128 liquidityForRA1 = uint128(FullMath.mulDiv(uint256(liquidityActual), receivedAmount1, amount1)); if (liquidityForRA1 < liquidityActual) { liquidityActual = liquidityForRA1; } } require(liquidityActual > 0, 'IIL2'); { (, int256 amount0Int, int256 amount1Int) = _updatePositionTicksAndFees(recipient, bottomTick, topTick, int256(liquidityActual).toInt128()); require((amount0 = uint256(amount0Int)) <= receivedAmount0, 'IIAM2'); require((amount1 = uint256(amount1Int)) <= receivedAmount1, 'IIAM2'); } if (receivedAmount0 > amount0) { TransferHelper.safeTransfer(token0, sender, receivedAmount0 - amount0); } if (receivedAmount1 > amount1) { TransferHelper.safeTransfer(token1, sender, receivedAmount1 - amount1); } emit Mint(msg.sender, recipient, bottomTick, topTick, liquidityActual, amount0, amount1); } function swap( address recipient, bool zeroToOne, int256 amountRequired, uint160 limitSqrtPrice, bytes calldata data ) external override returns (int256 amount0, int256 amount1) { uint160 currentPrice; int24 currentTick; uint128 currentLiquidity; uint256 communityFee; // function _calculateSwapAndLock locks globalState.unlocked and does not release (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice); if (zeroToOne) { if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient uint256 balance0Before = balanceToken0(); _swapCallback(amount0, amount1, data); // callback to get tokens from the caller require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA'); } else { if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient uint256 balance1Before = balanceToken1(); _swapCallback(amount0, amount1, data); // callback to get tokens from the caller require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA'); } if (communityFee > 0) { _payCommunityFee(zeroToOne ? token0 : token1, communityFee); } emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick); globalState.unlocked = true; // release after lock in _calculateSwapAndLock } /// @inheritdoc IAlgebraPoolActions function swapSupportingFeeOnInputTokens( address sender, address recipient, bool zeroToOne, int256 amountRequired, uint160 limitSqrtPrice, bytes calldata data ) external override returns (int256 amount0, int256 amount1) { // Since the pool can get less tokens then sent, firstly we are getting tokens from the // original caller of the transaction. And change the _amountRequired_ require(globalState.unlocked, 'LOK'); globalState.unlocked = false; if (zeroToOne) { uint256 balance0Before = balanceToken0(); _swapCallback(amountRequired, 0, data); require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA'); } else { uint256 balance1Before = balanceToken1(); _swapCallback(0, amountRequired, data); require((amountRequired = int256(balanceToken1().sub(balance1Before))) > 0, 'IIA'); } globalState.unlocked = true; uint160 currentPrice; int24 currentTick; uint128 currentLiquidity; uint256 communityFee; // function _calculateSwapAndLock locks 'globalState.unlocked' and does not release (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice); // only transfer to the recipient if (zeroToOne) { if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // return the leftovers if (amount0 < amountRequired) TransferHelper.safeTransfer(token0, sender, uint256(amountRequired.sub(amount0))); } else { if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // return the leftovers if (amount1 < amountRequired) TransferHelper.safeTransfer(token1, sender, uint256(amountRequired.sub(amount1))); } if (communityFee > 0) { _payCommunityFee(zeroToOne ? token0 : token1, communityFee); } emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick); globalState.unlocked = true; // release after lock in _calculateSwapAndLock }
liquidityDesired or amountRequired but these parameters are only used in output amount calculation. (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice);
It isn't used to prevent the output amounts from dropping below a threshold. As there aren't any require(amount1 >= ...) pattern before/after transferring the output token.
It is possible that the output amount is less than the amount desired due to this comment "the amount to get could be less then initially specified (e.g. reached limit)"
Add minAmountOut parameter and check it before sending the output token.
function mint( address sender, address recipient, int24 bottomTick, int24 topTick, uint128 liquidityDesired, uint128 minAmountOut, bytes calldata data ) external override lock onlyValidTicks(bottomTick, topTick) returns ( uint256 amount0, uint256 amount1, uint128 liquidityActual ) { require(liquidityDesired > 0, 'IL'); { (int256 amount0Int, int256 amount1Int, ) = _getAmountsForLiquidity( bottomTick, topTick, int256(liquidityDesired).toInt128(), globalState.tick, globalState.price ); amount0 = uint256(amount0Int); amount1 = uint256(amount1Int); } uint256 receivedAmount0; uint256 receivedAmount1; { if (amount0 > 0) receivedAmount0 = balanceToken0(); if (amount1 > 0) receivedAmount1 = balanceToken1(); IAlgebraMintCallback(msg.sender).algebraMintCallback(amount0, amount1, data); if (amount0 > 0) require((receivedAmount0 = balanceToken0() - receivedAmount0) > 0, 'IIAM'); if (amount1 > 0) require((receivedAmount1 = balanceToken1() - receivedAmount1) > 0, 'IIAM'); } liquidityActual = liquidityDesired; if (receivedAmount0 < amount0) { liquidityActual = uint128(FullMath.mulDiv(uint256(liquidityActual), receivedAmount0, amount0)); } if (receivedAmount1 < amount1) { uint128 liquidityForRA1 = uint128(FullMath.mulDiv(uint256(liquidityActual), receivedAmount1, amount1)); if (liquidityForRA1 < liquidityActual) { liquidityActual = liquidityForRA1; } } require(liquidityActual > 0 && liquidityActual >= minAmountOut, 'IIL2'); { (, int256 amount0Int, int256 amount1Int) = _updatePositionTicksAndFees(recipient, bottomTick, topTick, int256(liquidityActual).toInt128()); require((amount0 = uint256(amount0Int)) <= receivedAmount0, 'IIAM2'); require((amount1 = uint256(amount1Int)) <= receivedAmount1, 'IIAM2'); } if (receivedAmount0 > amount0) { TransferHelper.safeTransfer(token0, sender, receivedAmount0 - amount0); } if (receivedAmount1 > amount1) { TransferHelper.safeTransfer(token1, sender, receivedAmount1 - amount1); } emit Mint(msg.sender, recipient, bottomTick, topTick, liquidityActual, amount0, amount1); } function swap( address recipient, bool zeroToOne, int256 amountRequired, int256 minAmountOut, uint160 limitSqrtPrice, bytes calldata data ) external override returns (int256 amount0, int256 amount1) { uint160 currentPrice; int24 currentTick; uint128 currentLiquidity; uint256 communityFee; // function _calculateSwapAndLock locks globalState.unlocked and does not release (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice); if (zeroToOne) { require(amount1 < 0 && -amount1 >= minAmountOut, "IOA"); TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // transfer to recipient uint256 balance0Before = balanceToken0(); _swapCallback(amount0, amount1, data); // callback to get tokens from the caller require(balance0Before.add(uint256(amount0)) <= balanceToken0(), 'IIA'); } else { require(amount0 < 0 && -amount0 >= minAmountOut, "IOA"); TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // transfer to recipient uint256 balance1Before = balanceToken1(); _swapCallback(amount0, amount1, data); // callback to get tokens from the caller require(balance1Before.add(uint256(amount1)) <= balanceToken1(), 'IIA'); } if (communityFee > 0) { _payCommunityFee(zeroToOne ? token0 : token1, communityFee); } emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick); globalState.unlocked = true; // release after lock in _calculateSwapAndLock } /// @inheritdoc IAlgebraPoolActions function swapSupportingFeeOnInputTokens( address sender, address recipient, bool zeroToOne, int256 amountRequired, uint160 limitSqrtPrice, bytes calldata data ) external override returns (int256 amount0, int256 amount1) { // Since the pool can get less tokens then sent, firstly we are getting tokens from the // original caller of the transaction. And change the _amountRequired_ require(globalState.unlocked, 'LOK'); globalState.unlocked = false; if (zeroToOne) { uint256 balance0Before = balanceToken0(); _swapCallback(amountRequired, 0, data); require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA'); } else { uint256 balance1Before = balanceToken1(); _swapCallback(0, amountRequired, data); require((amountRequired = int256(balanceToken1().sub(balance1Before))) > 0, 'IIA'); } globalState.unlocked = true; uint160 currentPrice; int24 currentTick; uint128 currentLiquidity; uint256 communityFee; // function _calculateSwapAndLock locks 'globalState.unlocked' and does not release (amount0, amount1, currentPrice, currentTick, currentLiquidity, communityFee) = _calculateSwapAndLock(zeroToOne, amountRequired, limitSqrtPrice); // only transfer to the recipient if (zeroToOne) { require(amount1 < 0 && -amount1 >= minAmountOut, "IOA"); TransferHelper.safeTransfer(token1, recipient, uint256(-amount1)); // return the leftovers if (amount0 < amountRequired) TransferHelper.safeTransfer(token0, sender, uint256(amountRequired.sub(amount0))); } else { require(amount0 < 0 && -amount0 >= minAmountOut, "IOA"); TransferHelper.safeTransfer(token0, recipient, uint256(-amount0)); // return the leftovers if (amount1 < amountRequired) TransferHelper.safeTransfer(token1, sender, uint256(amountRequired.sub(amount1))); } if (communityFee > 0) { _payCommunityFee(zeroToOne ? token0 : token1, communityFee); } emit Swap(msg.sender, recipient, amount0, amount1, currentPrice, currentLiquidity, currentTick); globalState.unlocked = true; // release after lock in _calculateSwapAndLock }
#0 - trust1995
2022-10-01T21:38:21Z
I think "limitSqrtPrice" serves as adequate slippage protection for swaps.
#1 - 0xean
2022-10-02T23:50:45Z
given the lack of periphery contracts that would handle slippage for minting LP tokens, there doesn't appear to be any mechanism to ensure that during mint or burn there isn't greater than expected slippage. Will wait for the sponsor to comment on this to confirm my understanding.
If correct, this should be a H severity finding as it does lead to a loss of funds from sandwich attacks
#2 - vladyan18
2022-10-03T14:23:27Z
Contracts are initially made taking into account the use of appropriate periphery. Accordingly, slippage control is in the area of responsibility of the periphery.
#3 - 0xean
2022-10-03T15:14:57Z
given that this wasn't made explicit in the README and the scope of the contracts that are being audited is what was provided to the wardens, I believe the wardens issue to be valid.
#4 - sameepsi
2022-10-04T06:54:00Z
Yes, its handled in the periphery contracts. But, I will consider it to be a medium risk given we did not update the README file.
#5 - 0xean
2022-10-04T13:32:24Z
The issue is not that the README wasn't updated. The issue is that the contracts that were submitted for audit contain a vulnerability. The wardens spent time and energy to find that vulnerability and it should be awarded. It is good to know you already have a mitigation plan in place, but at the time of the wardens doing their work, they had no way of knowing that.
#6 - aktech297
2022-10-17T03:20:14Z
I believe it's safe for both user as well as for the protocol ...
#7 - 0xean
2022-10-19T12:34:09Z
After more discussions during QA, it was made clear that I missed the fact the the call from an EOA would revert. So a contract must be the caller here, which makes it less likely that this becomes an issue. While I do think the sponsors should have been more explicit in the readme, this brings down potential for this to be a problem considerably.
Given that it was not documented and an integrating contract would have to know that it needed to handle slippage will downgrade to M.
🌟 Selected for report: 0xNazgul
Also found by: 0x1f8b, 0x52, 0xDecorativePineapple, 0xSmartContract, 0xmatt, Aeros, Aymen0909, Bnke0x0, Chom, CodingNameKiki, Deivitto, DimitarDimitrov, IllIllI, JC, Jeiwan, Lambda, Matin, Migue, Mukund, Ocean_Sky, Olivierdem, RaymondFam, RockingMiles, Rolezn, Ruhum, Satyam_Sharma, Shinchan, Tomo, Trabajo_de_mates, V_B, Waze, __141345__, a12jmx, ajtra, asutorufos, aysha, brgltd, bulej93, carrotsmuggler, catchup, cccz, chrisdior4, cryptonue, cryptphi, d3e4, defsec, delfin454000, durianSausage, erictee, fatherOfBlocks, gogo, kaden, karanctf, ladboy233, lukris02, mahdikarimi, martin, mics, natzuu, oyc_109, p_crypt0, pedr02b2, rbserver, reassor, rotcivegaf, rvierdiiev, sikorico, slowmoses, sorrynotsorry, tnevler, trustindistrust
52.0694 USDC - $52.07
(amount0, amount1) = zeroToOne == cache.exactInput // the amount to provide could be less then initially specified (e.g. reached limit) ? (cache.amountRequiredInitial - amountRequired, cache.amountCalculated) // the amount to get could be less then initially specified (e.g. reached limit)
then should be than
(amount0, amount1) = zeroToOne == cache.exactInput // the amount to provide could be less than initially specified (e.g. reached limit) ? (cache.amountRequiredInitial - amountRequired, cache.amountCalculated) // the amount to get could be less than initially specified (e.g. reached limit)