Caviar contest - minhtrng's results

Lines of code

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

Vulnerability details

Impact

One of the assets (either baseTokens or fractionalTokens) will likely be overpaid when the user calls Pair.add liquidity.

Proof of Concept

The Pair.add function takes baseTokenAmount, fractionalTokenAmount and minLpTokenAmount as inputs. The first two parameters are pulled in from the user (or in the case of Ether as baseToken have to be sent with the transaction), while the latter is used as slippage control:

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

The addQuote function returns the amount of lp tokens minted, as a minimum of the baseTokenShare and fractionalTokenShare. In the add function both amounts are pulled in full though:

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

This will likely cause overpayment for one of the assets as described under #Impact, unless the user choses 0% slippage, in which case his tx will fail, if there are any swaps or liquidity changes happening before the transaction passes.

This could also possibly used in frontrunning attacks, though this has not been checked in-depth for a viable scenario due to time constraints.

Tools Used

Manual Review

Recommended Mitigation Steps

• Option 1: calculate excess tokens and send them back to the user
• Option 2: Only pull the funds necessary (as UniswapV2 does)

Lines of code

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

Vulnerability details

Impact

The buyQuote is not rounded up, which can cause a leak of value, due to the buyQuote being underestimated.

Proof of Concept

The function Pair.buyQuote does not round up, which can cause the issue described under #Impact:

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

Compare this to the equivalent function of UniswapV2Library:

function getAmountIn(uint amountOut, uint reserveIn, uint reserveOut) internal pure returns (uint amountIn) {
    require(amountOut > 0, 'UniswapV2Library: INSUFFICIENT_OUTPUT_AMOUNT');
    require(reserveIn > 0 && reserveOut > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY');
    uint numerator = reserveIn.mul(amountOut).mul(1000);
    uint denominator = reserveOut.sub(amountOut).mul(997);

    //[warden-note]: addition of 1 here is equivalent to rounding up
    amountIn = (numerator / denominator).add(1); 
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add 1 in the Pair.buyQuote function to round up:

return (outputAmount * 1000 * baseTokenReserves()) / ((fractionalTokenReserves() - outputAmount) * 997) + 1;