Caviar contest - nicobevi's results

Lines of code

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

Vulnerability details

Since there is no validation that the proportion between token1 and token2 remains the same when a liquidity provider adds more liquidity to the pool, there is a chance of losing funds for some liquidity providers.

The contract Pair.sol intends to function such as a normal ERC20 liquidity pool. However, if you look at other LP implementations you will find that there is no validations related to the tokens proportion added to the pool on this specific implementation. A liquidity provider could add any arbitriary amount of token1 and token2 calling the functon add().

As a liquidity provider, is expected that, if I add X amount of liquidity to the pool I should be able to get that same liquidity back if there is no changes on the pool (if there is no trades) minus the required fees if any.

However, on the current implementation where any arbitriary amount of pair tokens could be added to the pool without any validation, a second liquidity provider could add more liquidity in a different proportion (let's say that this second liquidity provider adds the same amount of token1 but half of token2) changing the underlying value of the lp token and losing funds if remove() is immediately called by this second provider.

Proof of concept:

1 - User1 adds 900 token1s and 900 tokens2s to the empty pool, getting their LP tokens. 2 - User2 adds 900 token1s and 900 toekn2s to the pool, getting their LP tokens. 3 - User2 calls remove() immediately reediming their LP tokens, but now their balance will be 600 token1s and 900 token2s. Losing 300 of their token1's initial balance. 4 - User1 calls remove() after this, getting their balance back, plus the lost token1 from user2, stealing their tokens.

  uint256 BTInitialBalance = 900;
  uint256 FTInitialBalance = 900;

  address user1 = makeAddr("user1");
  address user2 = makeAddr("user2");

  deal(address(usd), user1, BTInitialBalance, true);
  deal(address(p), user1, FTInitialBalance, true);

  deal(address(usd), user2, BTInitialBalance, true);
  deal(address(p), user2, FTInitialBalance, true);

  vm.startPrank(user1);
  usd.approve(address(p), type(uint256).max);
  p.approve(address(p), type(uint256).max);
  vm.stopPrank();

  vm.startPrank(user2);
  usd.approve(address(p), type(uint256).max);
  p.approve(address(p), type(uint256).max);
  vm.stopPrank();
  
  vm.startPrank(user1);
  uint256 user1BTBalance = usd.balanceOf(user1);
  uint256 user1FTBalance = p.balanceOf(user1);
  uint256 user1LpTokens = p.add(user1BTBalance, user1FTBalance, 0);
  vm.stopPrank();

  vm.startPrank(user2);
  uint256 user2BTBalance = usd.balanceOf(user2);
  uint256 user2FTBalance = p.balanceOf(user2);
  // USER2 WILL ADD HALF OF THE TOKEN2
  uint256 user2LpTokens = p.add(user2BTBalance, user2FTBalance / 2, 0); 
  vm.stopPrank();

  vm.prank(user2);
  p.remove(user2LpTokens, 0, 0);
  
  vm.prank(user1);
  p.remove(user1LpTokens, 0, 0);

  assertEq(usd.balanceOf(user1), BTInitialBalance, "THIS WILL FAIL - user1 base token balance");
  assertEq(p.balanceOf(user1), FTInitialBalance, "user1 fractional token balance");

  assertEq(usd.balanceOf(user2), BTInitialBalance, "THIS WILL FAIL - user2 base token balance");
  assertEq(p.balanceOf(user2), FTInitialBalance, "user2 fractional token balance");

Mitigation steps

It's important to add to the LP new tokens the tokens keeping the right proportion between them. If a lp provider tries to add liquidity in a different proportion, it's necessary to get just the right amount of the tokens and refund the remaining to the liquidity provider.