Caviar contest - carlitox477's results

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L95 https://github.com/code-423n4/2022-12-caviar/blob/0212f9dc3b6a418803dbfacda0e340e059b8aae2/src/Pair.sol#L137 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#L203

Vulnerability details

Description

Current implementation of functions add, remove, buy and sell first transfer fractional tokens, and then base tokens.

If this base token is ERC777 (extension of ERC20), we can call this function without updating the base token balance, but updating the fractional token balance.

Impact

Allows to drain funds of a pairs which implements an ERC-777 token.

POC

function buy(uint256 outputAmount, uint256 maxInputAmount) public payable returns (uint256 inputAmount) {
    // *** Checks *** //

    // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
    require(baseToken == address(0) ? msg.value == maxInputAmount : msg.value == 0, "Invalid ether input");

    // calculate required input amount using xyk invariant
+   @audit Use current balances
    inputAmount = buyQuote(outputAmount);

    // check that the required amount of base tokens is less than the max amount
    require(inputAmount <= maxInputAmount, "Slippage: amount in");

    // *** Effects *** //
+   @audit Modifies just fractional balance
    // transfer fractional tokens to sender
    _transferFrom(address(this), msg.sender, outputAmount);

    // *** Interactions *** //

    if (baseToken == address(0)) {
        // refund surplus eth
        uint256 refundAmount = maxInputAmount - inputAmount;
        if (refundAmount > 0) msg.sender.safeTransferETH(refundAmount);
    } else {

        // transfer base tokens in
+       @audit If an ERC-777 token is used, we can re call buy function with the same balance of base token, but with different fractional balance
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);

    }
    emit Buy(inputAmount, outputAmount);
}

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

The buy quote is used to calculate the amount of fractional token that the user will receive, and it should be less/equal to maxInputAmount sent by parameter in order to achieve a successful execution of function buy.

Current buy quote can be mathematically expressed as: $\frac{outputAmount \times 1000 \times baseTokenReserves}{fractionalTokenReserves - outPutAmount} \times 997$.

Then, about sales

function sell(uint256 inputAmount, uint256 minOutputAmount) public returns (uint256 outputAmount) {
    // *** Checks *** //

    // calculate output amount using xyk invariant
    outputAmount = sellQuote(inputAmount);

    // check that the outputted amount of fractional tokens is greater than the min amount
    require(outputAmount >= minOutputAmount, "Slippage: amount out");

    // *** Effects *** //

    // transfer fractional tokens from sender
+   //@audit fractional balance is updated
    _transferFrom(msg.sender, address(this), inputAmount);

    // *** Interactions *** //

    if (baseToken == address(0)) {
        // transfer ether out
        msg.sender.safeTransferETH(outputAmount);
    } else {
        // transfer base tokens out
+       @audit If an ERC-777 token is used, we can re call sell function with the same balance of base token, but with different fractional balance.
        ERC20(baseToken).safeTransfer(msg.sender, outputAmount);
    }

    emit Sell(inputAmount, outputAmount);
}

    uint256 inputAmountWithFee = inputAmount * 997;
    return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
}

Current sellQuote function can be expressed mathematically as:

$inputAmount = \frac{inputAmount \times 997 \times baseTokenReserves}{fractionalTokenReserves \times 1000 + inputAmountWithFee}$

Then we can think next scenario to drain a pair which use an ERC-777 token as base token:

1. Let's suppose the pair has 1000 base tokens(BT777) and 1000 Fractional reserve tokens (FRT)
2. The attacker call buy function, all with next inputs:
   • outputAmount = 50
   • maxInputAmount = 80
3. The attacker implements a hook, that will be executed 6 times (using a counter inside a malicus contract) when a transfer is done, and call the buy function. After this 6 times the malicious contract is call again, but this times calls the sell function, doing a huge sell for the fractional reserve token obtained.

A simulation of this attack can be visualized in next table

The result of this operation is that the attaker/malicious contract has 350 FRT, while BT777 reserve still has 1000 and FRT reserve has 650 tokens. The success execution needs that the attacker pays 442 BT777 eventually.

To do this, the last operation of the malicious contract is calling sell function

The result is that the attacker now controls 536 BT777, the attacker use this balance to pay the debt of 442 BT77, with a profit of 94 BT77 tokens.

Mitigation steps

Add openzeppelin nonReentrant modifier to mentioned functions, or state clear in the documentation that this protocol should not be used with ERC777 tokens.

Lines of code

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

Vulnerability details

Description

Current function assure as that we will at least receive minLpTokenAmount amount of LP. However, this function assumes that the input parameter baseTokenAmount and fractionalTokenAmount will adjust to the LP amount calculated to deposit.

Impact

A user whose input parameters baseTokenAmount or fractionalTokenAmount does not adjust to the corresponding lpTokenAmount calculated inside the function can make the user lose funds, or even open frontrunning opportunities for MEV bots.

POC

Add this file to test folder, then run forge test --match-test testUserLoseOfFunds -vvv to check the next output.

Logs:
  Current user 1 USD balance:  1000
  Current user 1 Fractional Token balance:  1000

User 1 deposits 100 USD and 150 fractional tokens
        User 1 USD balance after LP minting through add function:  900
        User 1 Fractional Token balance after LP minting through add function::  850
        User 1 LP balance:  100

User 1 tries to remove their LP to get their 100 USD and 150 fractional token
        User 1 USD balance after calling remove function with all their LPs:  1000
        User 1 Fractional Token balance calling remove function with all their LPs:  975
        User 1 LP balance:  0

This shows that a user calling this function can accidentally give tokens to current LP holders.

A derivate consequence is that it opens a windows to MEV bots, they can include in the same block a transaction to:

1. Mint an LP with correct amounts
2. Wait until the user who put wrong inputs and sent more token than they are supposed by accident
3. The bot now can remove the LP with more tokens.

This last scenario means that user funds are in risk.

Mitigation steps

Calculates amount of tokens/ETH to transfer to the Pair contract in fact and use these values to execute the transfer function and/or to return ETH. I would suggest to add this calculations inside the addQuote function. Then the return values can be even used in the front end to suggest user amount to send in order to the slippage they set.

-   function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
+   function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256, uint256, uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 0) {
            //@audit-ok 3 simple rules and return min is ok
            // calculate amount of lp tokens as a fraction of existing reserves
            // EXPECTED_SHARE = AMOUNT SENT * CURRENT SHARES / CURRENT BALANCE
+           uint256 _baseTokenReserve = baseTokenReserves(); 
+           uint256 _fractionalTokenReserves = fractionalTokenReserves(); 
            uint256 baseTokenShare = 
                (baseTokenAmount * lpTokenSupply) /
-               baseTokenReserves();
+               _baseTokenReserve;

            uint256 fractionalTokenShare = 
                (fractionalTokenAmount * lpTokenSupply) /
-               fractionalTokenReserves();
+               _fractionalTokenReserves;

+           if(baseTokenShare < fractionalTokenShare){
+               uint256 baseTokenToSend = _baseTokenReserve * baseTokenShare / lpTokenSupply;
+               uint256 fractionalTokenToSend = _fractionalTokenReserves * baseTokenShare / lpTokenSupply;
+               return baseTokenShare, baseTokenToSend, fractionalTokenToSend;
+           }else{
+               uint256 baseTokenToSend = _baseTokenReserve * fractionalTokenShare / lpTokenSupply;
+               uint256 fractionalTokenToSend = _fractionalTokenReserves * fractionalTokenShare / lpTokenSupply;
+               return fractionalTokenShare, baseTokenToSend, fractionalTokenToSend;
            }
-           return Math.min(baseTokenShare, fractionalTokenShare);
        } else {
            // if there is no liquidity then init
-           return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+           return Math.sqrt(baseTokenAmount * fractionalTokenAmount), baseTokenAmount, fractionalTokenAmount;
        }
    }

    function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
+      nonReentrant // consider adding this lock giving that we are sending back ETH to msg.sender, evaluate possible re-entrancy attacks
        returns (uint256 lpTokenAmount) 
    {
        // *** Checks *** //

        // check the token amount inputs are not zero
        require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");

        // check that correct eth input was sent - if the baseToken equals address(0) then native ETH is used
        require(baseToken == address(0) ? msg.value == baseTokenAmount : msg.value == 0, "Invalid ether input");

        // calculate the lp token shares to mint
-       lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
+       lpTokenAmount,baseTokenToSend, fractionalTokenToSend = addQuote(baseTokenAmount, fractionalTokenAmount);

        // check that the amount of lp tokens outputted is greater than the min amount
        require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");

        // *** Effects *** //
-       _transferFrom(msg.sender, address(this), fractionalTokenAmount);
+       _transferFrom(msg.sender, address(this), fractionalTokenToSend);

        // *** Interactions *** //

        // 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);
+           ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenToSend);
-       }
+       }else{
+            // return extra ETH
+           payable(msg.sender).call{value: msg.value - baseTokenToSend}()            
+       }

-       emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
+       emit Add(baseTokenToSend, fractionalTokenToSend , lpTokenAmount);
    }

Pair: Split require in order to save gas

Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 3 GAS per &&

// In Pair.add
-   require(baseTokenAmount > 0 && fractionalTokenAmount > 0, "Input token amount is zero");
+   require(baseTokenAmount > 0, ERROR_INPUT_TOKEN_AMOUNT_IS_ZERO);
+   require(fractionalTokenAmount > 0, ERROR_INPUT_TOKEN_AMOUNT_IS_ZERO);

Use _baseTokenReserves instead of baseTokenReserves function inside other functions

Internal functions spend less gass

//Pair.sol
399:    return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
408:    return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
417:    uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
437:    uint256 baseTokenOutputAmount = (baseTokenReserves() * lpTokenAmount) / lpTokenSupply;

Use balanceOf[address(this)] instead of fractionalTokenReserves function inside other functions

//Pair.sol
391:    return (_baseTokenReserves() * ONE) / fractionalTokenReserves();
399:    return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997);
408:    return (inputAmountWithFee * baseTokenReserves()) / ((fractionalTokenReserves() * 1000) + inputAmountWithFee);
422:    uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
438:    uint256 fractionalTokenOutputAmount = (fractionalTokenReserves() * lpTokenAmount) / lpTokenSupply;

Pair#withdraw: Cache closeTimestamp in order to avoid double access to storage variable

closeTimestamp is accessed twice, this can be avoided