[UNCHAIN-H#6] Depositors can be priced out and NFTs and baseToken can be stolen

[code423n4]

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.

[c4-judge]

berndartmueller marked the issue as duplicate of #442

[c4-judge]

berndartmueller marked the issue as satisfactory