Caviar contest - HE1M's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417

Vulnerability details

Impact

An attacker can steal liquidity provider's fund by manipulating the reserve.

During adding a liquidity, the amount of lpToken to be minted will be calculated in the function addQuote.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417

In this function, the minimum amount of baseTokenShare and fractionalTokenShare will be minted to the liquidity provider. Math.min(baseTokenShare, fractionalTokenShare)

If the amount which is going to be added are not balanced, then incorrect lpToken will be minted to the liquidity provider. This provides a critical attack surface for an attacker by making the reserve imbalance, so that incorrect lpToken to be minted to the liquidity provider.

For example for a pair with relation 1:3, an honest user is going to add 100 base token and 300 fractional token. But, if the attacker manipulates the reserve (by external transfer) so that the relation becomes 2:3, the honest user receives less lpToken. As a result, when removing the liquidity, the user will receive less fractional token.

Proof of Concept

• For simplicity, assume the pool is just created, so the reserve for both base token (USDC) and fractional token is zero. Moreover, no lpToken is minted yet.
• Bob (a malicious user) adds 1000 USDC and 1000 fractional token to the pair. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63
• So, totalSupply() = 1000, baseTokenReserves() = 1000, and fractionalTokenReserves() = 1000. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L426
• Then, Alice (an honest user) intends to add 2000 USDC and 2000 fractional tokens to the pair.
• In the following, I am analyzing two scenarios. The first one is for the normal working state of the pair (Alice's transaction is executed without any interruption), and the second one is when the attacker manipulates the reserve (Alice's transaction is front-run by Bob).

Safe (normal) scenario:

• When Alice's transaction is executed, the state of the pair will be:
  • baseTokenReserves() = 1000 + 2000 = 3000
  • fractionalTokenReserves() = 1000 + 2000 = 3000
  • Bob's lpToken = 1000
  • Alice's lpToken = min((2000*1000)/1000, (2000*1000)/1000) = 2000
  • totalSupply() = 1000 + 2000 = 3000
• It means that if Alice decides to remove liquidity from pair by burning her 2000 lpTokens, she will receive (2000*3000)/3000 = 2000 USDC and fractional tokens, which makes sense and is correct. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L107

Attack scenario:

• Bob notices the Alice's transaction in the Mempool and applies the front run attack. So, he transfers 3000 USDC directly to the contract pair. So, the state of the pair will be:
  • baseTokenReserves() = 1000 + 3000 = 4000
  • fractionalTokenReserves() = 1000
  • Bob's lpToken = 1000
  • Alice's lpToken = 0
  • totalSupply() = 1000
• Then, when Alice's transaction is executed, the sate will be:
  • baseTokenReserves() = 4000 + 2000 = 6000
  • fractionalTokenReserves() = 1000 + 2000 = 3000
  • Bob's lpToken = 1000
  • Alice's lpToken = min((2000*1000)/4000, (2000*1000)/1000) = 500
  • totalSupply() = 1000 + 500 = 1500
• The main difference between this scenario with the previous one is that Alice's lpToken is reduced to 1500 from 3000. This is done by Bob's reserve manipulation.
• It means that if Alice decides to remove liquidity from pair by burning her 500 lpTokens, she will receive (500*6000)/1500= 2000 USDC and (500*3000)/1500= 1000 fractional tokens. This is the vulnerability, because Alice added 2000 USDC and 2000 fractional tokens to the pair, but now she is receiving 1000 fraction token less than what is correct.
• If Bob decides to remove liquidity from pair by burning her 1000 lpTokens, he will receive (1000*6000)/1500= 4000 USDC and (1000*3000)/1500= 2000 fractional tokens. It means, Bob could steal 1000 fractional tokens from Alice.

Please note that the example scenario above is assuming that the pair does not have any fund, and everything is from scratch. But, it is possible to apply this attack even on an already nonempty pool.

Please note that this attack could be applied to steal base token (USDC) instead of fractional tokens. The only difference is that Bob should have transferred fractional token instead of base token directly to the pool.

Tools Used

Recommended Mitigation Steps

The problem is that when the reserves are imbalance, the protocol does not make it balance when a user is going to add liquidity to the pair. While, in other AMMs like Uniswap, the liquidity is balanced according to the reserves: https://github.com/Uniswap/v2-periphery/blob/0335e8f7e1bd1e8d8329fd300aea2ef2f36dd19f/contracts/UniswapV2Router02.sol#L48-L59

It makes sense that when adding liquidity (not for the first time), the price relation should not change, so a simple solution maybe is to add the following check:

function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {

        uint priceBefore = (_baseTokenReserves() * ONE) / fractionalTokenReserves();
        uint priceAfter = ((_baseTokenReserves() + baseTokenAmount) * ONE) / (fractionalTokenReserves() + fractionalTokenAmount);
        require(priceBefore == priceAfter, "imbalanced liquidity");
        
        // ...

    }

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L421

Vulnerability details

Impact

If a malicious user is the first person who is interacting with the pair, he can manipulate the reserve so that stealing fund from users if minLpTokenAmount == 0 or preventing users from adding liquidity if minLpTokenAmount != 0.

Proof of Concept

• Suppose a pair is just created.
• Bob (a malicious user) is the first person who is interacting with this pair.
• He gets some fractional tokens by wrapping his NFT. https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L217
• Then, Bob adds liquidity to the pair by calling the function add with the following parameters and providing the required baseToken which is USDC for example.
  • baseTokenAmount = 1
  • fractionalTokenAmount = 1
  • minLpTokenAmount = 1 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L63
• By doing so, 1 USDC and 1 fractional token will be transferred to the pair from Bob, and 1 lpToken will be minted to him based on the calculation done in the function addQuote. So, the totalSupply = 1.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L417 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L90

• Now Bob has the control of the pair and can apply two kinds of exploit:
  • First: if a user is going to add for example 1000 USDC and some fractional token with minLpTokenAmount parameter equal to zero, Bob front-runs the user and transfers directly to the pair 1000 USDC. By doing so, baseTokenReserves() = 1 + 1000 and total lpToken minted to the user will be equal to zero because of 1000 * 1 /(1 + 1000) = 0: uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves(); https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L421 Although, the user provided 1000 USDC, but zero lpToken is minted for the user. So, totalSupply = 1 and baseTokenReserves = 1 + 1000 + 1000. Now, Bob can remove his liquidity by burning his only lpToken, and receive 2001 USDC (1001 belongs to Bob, and 1000 USDC is user's fund). So, Bob could steal 1000 USDC from the user.
  • Second: if a user is going to add for example 1000 USDC and some fractional token with minLpTokenAmount parameter equal to nonzero X, Bob front-runs the user and transfers directly to the pair 1000/X USDC. By doing so, total lpToken minted to the user will be less than X because 1000 * 1 /(1 + 1000/X) = 1000 * X / (X + 1000) < X: uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves(); https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L421 As a result the user's transaction will be reverted because of: require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out"); https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L80 So, Bob could prevent any user from adding liquidity with nonzero minLpTokenAmount . Now, Bob can remove his liquidity by burning his only lpToken, and receive 1 + 1000/X USDC that belongs to him.

Tools Used

Recommended Mitigation Steps

• First Solution: The first person who is adding liquidity must add at least 100 USDC. So, totalSupply = 100 and baseTokenReserves() =100. By doing so, Bob (as the first person) must add 100 USDC and in return he receives 100 lpToken. To apply the attack when a user is adding 1000 USDC, Bob must transfer directly 99.9k + 1 USDC to be able to steal the user's 1000 USDC, because 1000 * 100 / (100 + Bob's direct transfer). So, Bob's attack requires a large amount of fund which makes it financially unreasonable.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            // calculate amount of lp tokens as a fraction of existing reserves
            uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
            uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
            return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            require(baseTokenAmount >= 100 * 10**6);
            // if there is no liquidity then init
            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
        }
    }

• Second Solution: It is recommended that when the pair is created a large amount of lpToken be minted to the pair. For example, if the protocol mints 1000 lpToken to the pair, Bob must provide almost 1,001,000 USDC to make this 1000 * (1000 + 1)/ (1 + Bob's direct transfer) equal to zero (after rounded down by EVM), which is almost impossible for Bob.