Caviar contest - minhquanym's results

Lines of code

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

Vulnerability details

It is important to be aware that I have reported two reentrancy bugs. Each of these have different ways of being activated and can be found in separate functions.

Impact

All calculations done in Caviar Pair are using token balance directly. For example, when users add liquidity, it will use baseToken.balanceOf(address(this)) to calculate the amount of LP token should be minted.

function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
    public
    payable
    returns (uint256 lpTokenAmount)
{
    ...
    // calculate the lp token shares to mint
    uint lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
    ...
    // mint lp tokens to sender
    lpToken.mint(msg.sender, lpTokenAmount);

    // transfer base tokens in if the base token is not ETH
    if (baseToken != address(0)) {
        // transfer base tokens in
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount); 
        // @audit ERC777 token reentrancy.
    }
    ...
}

However, if baseToken is ERC777 that implemented tokensToSend(), it could be exploited because the tokensToSend() hook is called before balance update.

Proof of Concept

Assuming baseToken is XXX - an ERC777 that implemnted tokensToSend(). This hook is called before balance update (Implementation Requirement from EIP-777)

• The token contract MUST call the tokensToSend hook before updating the state.

Consider the scenario:

1. At first, liquidity pool has 1000 XXX - 1000 FracToken and total supply of LP token is 1000.
2. Alice (attacker contract) calls add() to add 100 XXX and 100 FracToken. The calculate and mint lpTokenAmount = 100 to Alice. At this moment, liquidity pool has 1000 XXX (because baseToken is not transferred in yet) and 1100 FracToken and total supply of LP token is 1100.
3. After minting, during safeTransferFrom() calls, Alice received a tokensToSend() hook. She used this to call add() again with 100 XXX and 110 FracToken. Now lpTokenAmount is calculated as

baseTokenShare = 100 * 1100 / 1000 = 110
fractionalTokenShare = 110 * 1100 / 1100 = 110
lpAmount = min(baseTokenShare, fractionalTokenShare) = 110

So totally, Alice added 200 XXX and 210 FracToken and she received 210 LP token while she is expected to only receive 200 LP token normally.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding nonReentrant modifier to these functions to protect them from reentrancy issue.

Lines of code

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

Vulnerability details

Impact

Function Pair.add() receives base token and fractional token from liquidity providers and mint equivalent amount of LP token for them.

The amount of LP token be minted is calculate in 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);
    }
}

As we can see, it takes the min value between baseTokenShare and fractionalTokenShare as the return value. However, the surplus token is not returned to sender but lock in the liquidity pool and become assets of all LPs, which means sender is losing funds.

Proof of Concept

Consider the scenario

1. Currently, liquidity pool has 100 USDC - 100 FracToken. Total supply of LP token is 100.
2. Alice want to add 50 USDC - 50 FracToken. She should get 33.33% of total supply of LP token, which is 50 LP token.
3. Bob (attacker) front-run Alice's transaction, he swap to make the pool become 90 USDC - 111.11 FracToken.
4. Now Alice will receive only 45 instead of 50 LP token.

baseTokenShare = 50 * 100 / 90 = 55.55
fractionalTokenShare = 50 * 100 / 111.11 = 45
lpAmount = min(baseTokenShare, fractionalTokenShare) = 45

There is 55.55 - 45 = 10.55 surplus base token share. It should be return to Alice.

Tools Used

Manual Review

Recommended Mitigation Steps

• In Uniswap V2, users have a router to help calculate LP token amount on-chain before sending token in. So users will not send more than needed if they use router contract.
• Since Caviar does not have router contract to support, consider returning surplus amount to LP.

Lines of code

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

Vulnerability details

Impact

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept

In Pair.add(), the amount of LP token minted is calculated as

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

An attacker can exploit using these steps

1. Create and add 1 wei baseToken - 1 wei quoteToken to the pair. At this moment, attacker is minted 1 wei LP token because sqrt(1 * 1) = 1
2. Transfer large amount of baseToken and quoteToken directly to the pair, such as 1e9 baseToken - 1e9 quoteToken. Since no new LP token is minted, 1 wei LP token worths 1e9 baseToken - 1e9 quoteToken.
3. Normal users add liquidity to pool will receive 0 LP token if they add less than 1e9 token because of rounding division.

baseTokenShare = (X * 1) / 1e9;
fractionalTokenShare = (Y * 1) / 1e9;

Tools Used

Manual Review

Recommended Mitigation Steps

• Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when lpTokenSupply == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.
• In add(), ensure the number of LP tokens to be minted is non-zero:

require(lpTokenAmount != 0, "No LP minted");

Summary

1. High price NFT will be unwrap immediately, replaced by a lower value NFT

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

Detail

It is evident that with wrap()/unwrap(), an individual can exchange a lower-valued Non-Fungible Token (NFT) for a higher-valued one within the same collection. This is because each NFT is unique, and thus, usually has its own distinct value/price.

The result of this design will be that only one (or a few) NFTs with the lowest value will remain in the Pair. This is because if someone wraps an NFT which has a price slightly higher than the floor price, other traders will immediately trade it for one at the floor price.

2. Unwrap did not check whether tokenId is in merkle proof.

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

Detail

If an NFT is mistakenly directly sent to the Pair contract not included in merkle proof, the FracToken can be used to withdraw it with unwrap() function.

Consider the scenario where this NFT has higher value than all NFTs in merkle proof, it should be withdrawn by admin to possibly return to true owner.