Caviar contest - yixxas's results

Lines of code

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

Vulnerability details

Impact

This is a common vulnerability. The token supply can be manipulated to prevent users from gaining a "normal" number of shares. See Section 3.4 Uniswap does this by burning the first 1000 lpTokens to significantly increase the cost of this attack.

Proof of Concept

First depositor to liquidity pool can deposit a small amount, i.e. 1 wei to mint 1 LP token. They can then donate a large number of baseToken to the protocol and make it difficult for small liquidity providers from minting any LP tokens.

baseTokenShare is calculated as baseTokenAmount * lpTokenSupply / baseTokenReserves(). Because if lpTokenSupply is 1, and if a malicious user donates 100000e18 baseToken, the next depositors need to provide at least 100000e18 baseToken to mint 1 lpToken. If user deposits say 150000e18, they will still only mint 1 lpToken due to the large rounding error.

    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);
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

We can do what Uniswap does, by sending the first 1000 lptoken to the 0 address when liquidity is first initiated.

Lines of code

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

Vulnerability details

Impact

decimals of baseToken is not accounted for. It is always assumed to be the same as in fractionalToken. Should a coin like USDC be used as the baseToken, NFTs can be easily purchased for free.

Note that the same mistake is made in sellQuote(), hence affected functions are buy(), sell(), add(), remove() as they all assume that decimals of baseToken and fractionalToken are the same.

Proof of Concept

inputAmount is calculated as such. This value is used to determine how much a user has to pay in buy().

     function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

If baseToken is a coin like USDC which is 6 decimals, our computation here will almost always return 0, or a much lesser amount than it should be due to the massive difference in decimals i.e. 6 vs 18.

On the other hand, if basetoken decimal is more than 18 decimals, buyers would be overpaying the difference.

Tools Used

Manual Review

Recommended Mitigation Steps

For example in buyQuote(), our calculation should scale to e18, which is the same as fractionalTokenReserve().

     function buyQuote(uint256 outputAmount) public view returns (uint256) {
-        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
+        return (outputAmount * 1000 * baseTokenReserves() * 1e18 / baseToken.decimals() ) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

Lines of code

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

Vulnerability details

Impact

It is possibly for buyQuote() to quote an input amount of 0 despite setting an output amount > 0. This means that an attacker can constantly steal from the protocol by calling buy() with a small input amount each time.

Proof of Concept

inputAmount is calculated as follows.

    function buyQuote(uint256 outputAmount) public view returns (uint256) {
        return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
    }

If baseTokenReserves() > fractionalTokenReserves(), this attack can happen.

For eg, if fractionalTokenReserves() = 100, baseTokenReserves() = 1000, outputAmount = 9

inputAmount = buyQuote(9) = 9 * 1000 * 1000 / ( (100 - 9) * 997 ) = 0

inputAmount is used in buy() to determine how much user has to pay. In this case, attacker can drain 9 baseTokens from the protocol without having to pay any fractionalToken.

Tools Used

Manual Review

Recommended Mitigation Steps

We should add the check in buy(), require( inputAmount != 0 ) to prevent this from happening.

L01 - Array input length are not checked to be the same

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

wrap() takes in tokenIds[] and proofs[][] but their lengths are not checked to be the same. It is recommended that we check length to be equal to avoid unintended actions by the users.

L02 - Solmate's safeTransferFrom does not check for code existence

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

In the event that NFT contract gets temporarily destroyed, users can mint fractionalToken for free since the safeTransferFrom call would always succeed even if users to do not own the required NFTs.

Q01 - Use the special ETH burn address to represent when baseToken is ETH

Instead of setting baseToken to address(0) to represent baseToken is ETH, we can consider using the vanity address 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE to represent this. This makes it much clearer that we are using ETH as the baseToken.