Surplus token after adding liquidity is not refunded to liquidity providers. LP might suffer front-running attack and lose funds.

[code423n4]

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.

[Shungy]

Dup #90

[c4-judge]

berndartmueller marked the issue as duplicate of #376

[c4-judge]

berndartmueller marked the issue as satisfactory