QuickSwap and StellaSwap contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L416-L485

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

AlgebraPool.sol#L227-L230

uint32 _liquidityCooldown = liquidityCooldown;
if (_liquidityCooldown > 0) {
    require((_blockTimestamp() - lastLiquidityAddTimestamp) >= _liquidityCooldown);
}

The code above from AlgebraPool.sol#_recalculatePosition causes a revert if the liquidity has been added or removed with in the specified cooldown period. _recalculatePosition is called whenever a position is minted (added to) or burned (removed from). Mint allows a user to add liquidity to a different user, which allows an adversary to DOS a specific user from depositing or withdrawing their liquidity. The adversary would front run the mint or burn transaction with a malicious mint for a small amount. This would initiate the cooldown period causing the user's deposit or withdraw to fail.

Concentrated liquidity positions are particularly vulnerable to impermanent loss. An adversary could take advantage of this to block the user causing greater impermanent loss to a user that is trying to withdraw a position. This would mean that this DOS vector timed correctly could lead to loss of funds for the user being DOS'd.

Tools Used

Recommended Mitigation Steps

I don't see any reason why mint needs to allow specifying a different recipient. It should be remove to resolve this DOS vector

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L891-L949

Vulnerability details

Impact

Flashloan users forced to pay higher fees than expected

Proof of Concept

The first swap of the block sets the fee that will apply to all other actions during that block, including the fee that will be applied to flashloans. If a swap occurs in the same block before the flashloan, the fee taken from the flashloan will be different than expected potentially much different. This could be used by an adversary to force a flashloan user to pay higher fees. They would frontrun the flashloan call with a swap that would push the fees higher. This is dependent on the current state of the swap beforehand but liquidity providers would have a strong incentive to monitor the pool to maximize their profits.

Tools Used

Manual Review

Recommended Mitigation Steps

Using the swap fee as the flashloan fee seems counterintuitive to me. It doesn't cause any impermanent loss to the liquidity providers, which is what the dynamic fee is designed to offset. There are two possible solutions to this. The first solution would be to allow the flashloan user to specify a max fee they will pay. The other solution, which I believe makes more sense, is to just use a flat fee on flashloans. This provides more consistent outcomes for flashloan users, making the product more attractive.

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L626-L673

Vulnerability details

Impact

Exact output functionality does not exist for fee on transfer tokens

Proof of Concept

require((amountRequired = int256(balanceToken0().sub(balance0Before))) > 0, 'IIA');

Exact output swaps are signaled by setting amountRequired to a negative number. When calling AlgebraPool#swapSupportingFeeOnInputTokens amount required is set to the difference is token balances before and after which will always be positive. Since amountRequired can't be negative, the exact output functionality is impossible to use for those tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Fee on transfer tokens are messy and there is no standard implementation to query the fee from the token contract. Ultimately support for these tokens should be added through either user inputs (i.e. allowing high slippage) or a specialized router

Lines of code

https://github.com/code-423n4/2022-09-quickswap/blob/15ea643c85ed936a92d2676a7aabf739b210af39/src/core/contracts/AlgebraPool.sol#L626-L673

Vulnerability details

Impact

User is forced to pay token transfer fees twice, once when taking payment and once when returning extra

Proof of Concept

Fee on transfer tokens take a fee each time the token is transferred. AlgebraPool#swapSupportingFeeOnInputTokens implements two transfers, one for the main payment and one for the refund. This causes the users to have to pay token transfer fees twice.

Tools Used

Recommended Mitigation Steps

Support for fee on transfer tokens should be added through either user inputs (i.e. allowing high slippage) or a specialized router