Caviar contest - UNCHAIN's results

Lines of code

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

Vulnerability details

Finder

Tomo

Vulnerability Detail

In add(), users can get the amount of lpToken. This amount is the lesser of baseToken and fractionalToken share.

There is a check to prevent the amount of lpToken is greater than the minOutputAmount but there is no check for the amount of tokens lost by the one with the larger share.

PoC

lpTokenTotalSupply = 100e18, baseTokenReserves() = 10e18, fractionalTokenReserves() = 50e18

1. Alice execute add() with 1 USDC and 1e18 fractionalToken
2. In the addQuote(), the baseTokenShare and fractionalTokenShare would be like this.

// calculate amount of lp tokens as a fraction of existing reserves
// baseTokenShare = 1e6 * 100e18 / 10e18 = 1e7
uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
// fractionalTokenShare = 1e18 * 100e18 / 50e18 = 2e18
uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();

1. The addQuote() would return the baseTokenShare as a return value
2. Finally, Alice got the 1e7 lpToken with 1 USDC and 1e18 fractionalToken
3. Alice can get the same amount of lpToken with 1 USDC and 5e6 fractionalToken

Code Snippet

function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
        public
        payable
        returns (uint256 lpTokenAmount)
    {
        // *** Checks *** //
				/* ... */
        lpTokenAmount = 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 *** //

				// transfer fractional tokens in
        _transferFrom(msg.sender, address(this), fractionalTokenAmount);

				/* ... */

        lpToken.mint(msg.sender, lpTokenAmount);

				/* ... */

        if (baseToken != address(0)) {
            // transfer base tokens in
            ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
        }

        emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
    }

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

Tool used

Manual

Recommendation

For a token amount with a large share, only the amount of the share that is the same as that of the smaller share should be charged in the transfer.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L58-L99

Vulnerability details

Finder

Yawn, Tomo, keit

Summary

The Add function allows the user to add any amount to the Pair contract.Both the add and remove functions use the balance of the contract’s baseToken in their calculations for shares.

This allows attacker to tie the share to very high amounts, and steal fractionalToken and baseToken by pricing out smaller depositors.

Vulnerability Detail

• Attacker execute the nftAdd function with arguments so that lpTokenAmount is close to the minimum amount 1.
• Attacker transfer 1000 USDC directly to the Pair contract. The baseToken’s balanceOf() is now 1000e6 + 1
• If any other depositors make a deposit of 1000 or less baseToken, the share will be set to 0.baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves() = (1000e6 * 1) / (1000e6 + 1) = 0
• Also, even if the user purchases fractional token with the buy function and provides liquidity with the add function, the share will be zero because the smaller share is calculated as the amount of the lpToken.return Math.min(baseTokenShare, fractionalTokenShare);
• Attacker can call remove as lpTokenAmount = 1 to receive the entire Pair balance both of baseToken and fractional tokens. So attacker can steal all of the NFTs and baseToken balance.

Code Snippet

function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
    publicpayablereturns (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);

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

    // *** Effects *** //

    // transfer fractional tokens in
    _transferFrom(msg.sender, address(this), fractionalTokenAmount);

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

    emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
}

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

Tool used

Manual Review

Recommendation

It is recommended to define a minimum deposit amount as UniswapV2 does, and mint the initial shares like 1000 to the zero address.

[yawn H-02] The value of the lpToken could be manipulated by sending baseToken directly.

Summary

Since the actual baseToken and fractionalToken balance in the Pair contract is used to calculate the lpToken value and fractionalToken price, they can be manipulated by transferring directly to the Pair contract.Under some conditions, users’ share may be estimated lower than expected and loses value of assets.

Vulnerability Detail

• Attacker gains more than majority share of the low liquid Pair by add function.
• Attacker sends a large number of baseToken directly to the Pair contract.
• The price of fractionalToken is calculated by xy*k, so it sets higher.
• If any other depositors make a deposit of baseToken, the share will be smaller than expected.
• Also, even if the user provides fractionalToken with the add function, the share will be smaller than expected when the amount of baseToken to provide has a small impact on the liquidity of the Pair.return Math.min(baseTokenShare, fractionalTokenShare);
• The Attacker can call remove to receive more baseTokens and fractionalTokens than expected.

Code Snippet

function _baseTokenReserves() internal view returns (uint256) {
    return baseToken == address(0)
        ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
        : ERC20(baseToken).balanceOf(address(this));
}

function fractionalTokenReserves() public view returns (uint256) {
    return balanceOf[address(this)];
}

Tool used

Manual Review

Recommendation

It is recommended that the baseToken and fractionalToken balances in the Pair contract be saved as storage variables like reserve each time they are updated, as is done in UniswapV2.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L147

Vulnerability details

Finder

Yosuke

Vulnerability details

Impact

An attacker can steal a ton of fractional tokens in the Pair contract.

Proof of Concept

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L147 ・Suppose there are 1 NFTs of BAYC, 80000 USDC, and 1e18 fractional tokens in a Pair contract and anyone hasn't called remove() function, buy() function, nftRemove() function, and nftBuy() function yet. ・Attacker puts a small number like 10000 in the first and second arguments and calls the buy() function.

buy(10000, 10000);

・The formula (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997) is used when calculating the amount of base tokens in the buyQuote() function.

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

・fractionalTokenReserves() is 10 to the 18th power, which is a very big number.

fractionalTokenReserves() = 1*1e18

・Applying the above formula, we get

return (10000 * 1000 * 80000) / ((1 * 1e18 - 10000) * 997)

・((1 * 1e18 - 10000) * 997) is greater than (10000 * 1000 * 80000) and the denominator is greater than the numerator, so the quotient is 0 by the solidity language specification.

(10000 * 1000 * 80000) < ((1 * 1e18 - 10000) * 997)
so,
return 0

・When the buyQuote() function returns 0, the attacker can get 10000 fractional tokens without paying any base tokens（ERC20）.

・And to make matters worse, as long as the numerator of the above formula is not greater than the denominator, the attacker can steal the fractional token any number of times.

・If the attacker has a ton of fractional tokens, he can exchange the fractional tokens for base tokens or buy NFTs from the Pair contract.

Recommended Mitigation Steps

Add a check for the return value from buyQuote() and revert if the return value is 0. https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L155

before

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
    inputAmount = buyQuote(outputAmount);

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

    // *** Effects *** //

    // 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
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
    }

    emit Buy(inputAmount, outputAmount);
}

after

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
    inputAmount = buyQuote(outputAmount);

    if( inputAmount == 0 )revert("inputAmount is 0")// add

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

    // *** Effects *** //

    // 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
        ERC20(baseToken).safeTransferFrom(msg.sender, address(this), inputAmount);
    }

    emit Buy(inputAmount, outputAmount);
}

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L379-L381 https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L383-L385

Vulnerability details

Finder

yawn

Summary

Since the actual baseToken and fractionalToken balance in the Pair contract is used to calculate the lpToken value and fractionalToken price, they can be manipulated by transferring directly to the Pair contract.Under some conditions, users’ share may be estimated lower than expected and loses value of assets.

Vulnerability Detail

• Attacker gains more than majority share of the low liquid Pair by add function.
• Attacker sends a large number of baseToken directly to the Pair contract.
• The price of fractionalToken is calculated by xy*k, so it sets higher.
• If any other depositors make a deposit of baseToken, the share will be smaller than expected.
• Also, even if the user provides fractionalToken with the add function, the share will be smaller than expected when the amount of baseToken to provide has a small impact on the liquidity of the Pair.return Math.min(baseTokenShare, fractionalTokenShare);
• The Attacker can call remove to receive more baseTokens and fractionalTokens than expected.

Code Snippet

function _baseTokenReserves() internal view returns (uint256) {
    return baseToken == address(0)
        ? address(this).balance - msg.value // subtract the msg.value if the base token is ETH
        : ERC20(baseToken).balanceOf(address(this));
}

function fractionalTokenReserves() public view returns (uint256) {
    return balanceOf[address(this)];
}

Tool used

Manual Review

Recommendation

It is recommended that the baseToken and fractionalToken balances in the Pair contract be saved as storage variables like reserve each time they are updated, as is done in UniswapV2.

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/main/src/Pair.sol#L447-L459

Vulnerability details

Finder

Tomo

Vulnerability Detail

The _transferFrom() returns variables as a return value. This value shows whether executing this transaction is a success or not.

However, some functions use _transferFrom() has no check for this return value.

This will leads to the user does not know why the transaction failed or lead to an unexpected error.

Code Snippet

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

        // *** Effects *** //

        // transfer fractional tokens in
        _transferFrom(msg.sender, address(this), fractionalTokenAmount);

}

function _transferFrom(address from, address to, uint256 amount) internal returns (bool) {
        balanceOf[from] -= amount;

        // Cannot overflow because the sum of all user
        // balances can't exceed the max uint256 value.
        unchecked {
            balanceOf[to] += amount;
        }

        emit Transfer(from, to, amount);

        return true;
    }

Tool used

Manual Review

Recommendation

Consider adding a check for the success of executing _transferFrom

require(_transferFrom(/* ... */),"Failed transferFrom");