Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimum liquidity pool share may become infeasible for small liquidity providers to provide any liquidity #88

Closed
code423n4 opened this issue Dec 15, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-442 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge

Comments

@code423n4
Copy link
Contributor

code423n4 commented Dec 15, 2022

Lines of code

https://github.com/code-423n4/2022-12-caviar/blob/d42a53b569ee06254ec3b5fd17ca2e527592dfe4/src/Pair.sol#L426

Vulnerability details

Impact

Pair.addQuote calculates the amount of LP tokens received for adding a given amount of base tokens and fractional tokens. If there are no existing deposits, then initializes to Math.sqrt(baseTokenAmount * fractionalTokenAmount). The problem arises when the value of a liquidity pool share grows over time, either by accumulating trading fees or through “donations” to the liquidity pool. In theory, this could result in a situation where the value of the minimum quantity of liquidity pool shares (1e-18 pool shares) is worth so much that it becomes infeasible for small liquidity providers to provide any liquidity. (See here on Uniswap v2 whitepaper). Creating MINIMUM_LIQUIDITY tokens and sending them to address zero to lock them also makes sure that they can never be redeemed, which means the pool will never be emptied completely, and this saves us from division by zero in some places (See Ethereum.org contract walkthrough).

Proof of Concept

With time, the worth of the minimum quantity of liquidity pool share (i.e. 1**-18), becomes high. High enough that the small liquidity providers will be unable to provide liquidity.

Tools Used

Manual inspection

Recommended Mitigation Steps

To mitigate this issue, Uniswap v2 burns the first 1e-15 (0.000000000000001) pool shares that are minted (1000 times the minimum quantity of pool shares), sending them to the zero address instead of to the minter. This should be a negligible cost for almost any token pair. But it dramatically increases the cost of the above attack. In order to raise the value of a liquidity pool share to 100 USD, the attacker would need to donate 100,000 USD to the pool, which would be permanently locked up as liquidity.

diff --git a/src/Pair.sol b/src/Pair.sol
index 185d25c..73bb3ff 100644
--- a/src/Pair.sol
+++ b/src/Pair.sol
@@ -19,6 +19,7 @@ contract Pair is ERC20, ERC721TokenReceiver {
 
     uint256 public constant ONE = 1e18;
     uint256 public constant CLOSE_GRACE_PERIOD = 7 days;
+    uint256 public constant MINIMUM_LIQUIDITY = 1000;
 
     address public immutable nft;
     address public immutable baseToken; // address(0) for ETH
@@ -86,6 +87,9 @@ contract Pair is ERC20, ERC721TokenReceiver {
 
         // *** Interactions *** //
 
+        if(lpToken.totalSupply() == 0) {
+            lpToken.mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
+        }
         // mint lp tokens to sender
         lpToken.mint(msg.sender, lpTokenAmount);
 
@@ -423,7 +427,7 @@ contract Pair is ERC20, ERC721TokenReceiver {
             return Math.min(baseTokenShare, fractionalTokenShare);
         } else {
             // if there is no liquidity then init
-            return Math.sqrt(baseTokenAmount * fractionalTokenAmount);
+            return Math.sqrt(baseTokenAmount * fractionalTokenAmount) - MINIMUM_LIQUIDITY;
         }
     }
 
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 15, 2022
code423n4 added a commit that referenced this issue Dec 15, 2022
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as duplicate of #442

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label Jan 10, 2023
@c4-judge
Copy link
Contributor

berndartmueller changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge labels Jan 10, 2023
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-442 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards upgraded by judge Original issue severity upgraded from QA/Gas by judge
Projects
None yet
Development

No branches or pull requests

2 participants