Caviar contest - __141345__'s results

Lines of code

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

Vulnerability details

Impact

When users provide LP, the exact amounts required for baseToken and fractionalToken could vary depends on many factors, such as former txn swap in the same block. It is quite possible that the provided baseTokenAmount and fractionalTokenAmount are not perfectly proportional to the reserves. But the add() function just transfer whatever as input, the extra token will not be returned.

As a result:

• users could lose some portion of the fund.
• the underlying reserve amount will deviate from x * y = k curve, the price for future trades will be off.

Proof of Concept

The add() function will transfer whatever amount provided, even if the amount are not proportional to the reserves.

File: src/Pair.sol
63:     function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
64:         public
65:         payable
66:         returns (uint256 lpTokenAmount)
67:     {

76:         // calculate the lp token shares to mint
77:         lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
78: 
79:         // check that the amount of lp tokens outputted is greater than the min amount
80:         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
81: 
82:         // *** Effects *** //
83: 
84:         // transfer fractional tokens in
85:         _transferFrom(msg.sender, address(this), fractionalTokenAmount);
86: 
87:         // *** Interactions *** //
88: 
89:         // mint lp tokens to sender
90:         lpToken.mint(msg.sender, lpTokenAmount);
91: 
92:         // transfer base tokens in if the base token is not ETH
93:         if (baseToken != address(0)) {
94:             // transfer base tokens in
95:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
96:         }
97: 
98:         emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
99:     }


417:     function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
418:         uint256 lpTokenSupply = lpToken.totalSupply();
419:         if (lpTokenSupply > 0) {
420:             // calculate amount of lp tokens as a fraction of existing reserves
421:             uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
422:             uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
423:             return Math.min(baseTokenShare, fractionalTokenShare);
424:         } else {
425:             // if there is no liquidity then init
426:             return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
427:         }
428:     }

Imagine the following: the pool begins with DAI 1,000, NFTToken 1,000, lpToken totalSupply 1,000, price is 1.0000.

• Alice wants to provide LP with 100 DAI and 100 NFTToken, `add(100, 100, 70).
• In the same block, the txn before Alice's call swaps 254 DAI for 200 NFTToken. The pool becomes DAI 1254, NFTToken 800, lpToken totalSupply 1,000. Price is 1.5675.
• Alice will receive 79 lpToken, but all 100 DAI and 100 NFTToken is transferred.
• The pool now has DAI 1354, NFTToken 900, price becomes 1.5044.

If recalculate by the actual reserves, the amount should be 100 DAI and 63 NFTToken. The difference 37 NFTToken is lost to the pair.

Also, the AMM price of the 2 token should not vary due to newly added liquidity.

The numbers used above is exaggerate for slippage, but the idea persists, that in-proportional amounts could result in user loss.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• return the extra token deviates from the current reserves.
• maybe add some parameter on the price, such as baseTokenReserves()/fractionalTokenReserves(), if the ratio of the amount used (baseTokenAmount/fractionalTokenAmount) differ too much from the spot price, just revert the transaction. Similar idea to the slippage control.

Lines of code

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

Vulnerability details

Impact

When a new pair is created, a malicious user can front run mint the first LP, and the price can be arbitrarily low or high, regardless of the class being floor or rare.

Later LP providers could mistakenly provide liquidity at the price set by the malicious actor, allowing the malicious user to swap tokens at the incorrect price and extract profit.

Proof of Concept

When the lpTokenSupply is 0, the price can be set to arbitrary value. Later LP providers can only follow the price.

File: src/Pair.sol
417:     function addQuote(uint256 baseTokenAmount, uint256 fractionalTokenAmount) public view returns (uint256) {
418:         uint256 lpTokenSupply = lpToken.totalSupply();
419:         if (lpTokenSupply > 0) {
420:             // calculate amount of lp tokens as a fraction of existing reserves
421:             uint256 baseTokenShare = (baseTokenAmount * lpTokenSupply) / baseTokenReserves();
422:             uint256 fractionalTokenShare = (fractionalTokenAmount * lpTokenSupply) / fractionalTokenReserves();
423:             return Math.min(baseTokenShare, fractionalTokenShare);
424:         } else {
425:             // if there is no liquidity then init
426:             return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
427:         }
428:     }

But the transferred amount of baseToken and fractionalToken remain the same, which are not sensitive to the ratio of underlying reserves.

063:     function add(uint256 baseTokenAmount, uint256 fractionalTokenAmount, uint256 minLpTokenAmount)
064:         public
065:         payable
066:         returns (uint256 lpTokenAmount)
067:     {
068:         // *** Checks *** //

076:         // calculate the lp token shares to mint
077:         lpTokenAmount = addQuote(baseTokenAmount, fractionalTokenAmount);
078: 
079:         // check that the amount of lp tokens outputted is greater than the min amount
080:         require(lpTokenAmount >= minLpTokenAmount, "Slippage: lp token amount out");
081: 
082:         // *** Effects *** //
083: 
084:         // transfer fractional tokens in
085:         _transferFrom(msg.sender, address(this), fractionalTokenAmount);
086: 
087:         // *** Interactions *** //
088: 
089:         // mint lp tokens to sender
090:         lpToken.mint(msg.sender, lpTokenAmount);
091: 
092:         // transfer base tokens in if the base token is not ETH
093:         if (baseToken != address(0)) {
094:             // transfer base tokens in
095:             ERC20(baseToken).safeTransferFrom(msg.sender, address(this), baseTokenAmount);
096:         }
097: 
098:         emit Add(baseTokenAmount, fractionalTokenAmount, lpTokenAmount);
099:     }
100: 

Imagine the following:

• Alice creates a BAYC-DAI pool for rare class, intending to set price as 100 DAI per NFT
• Alice tries to call add(10,00*1e18, 10*1e18, 100*e18)
• Bob front-runs Alice's txn with add(10,000, 1, 100), setting a price such that 1 NFT = 10,000 DAI.
• Alice still get LP amount of 100*e18 and deposits $1,000 of DAI, but only 0.1 DAI worth NFT goes to the LP
• Bob swaps DAI for the 999.9 DAI value NFT.

The above can also go in the other direction, to set the price extremely high, the idea is analogous.

Tools Used

Manual analysis.

Recommended Mitigation Steps

Enforced the pair creator to make the first deposit with proper initial price.