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

First depositor can break minting of shares #442

Open
code423n4 opened this issue Dec 19, 2022 · 4 comments · Fixed by outdoteth/caviar#3
Open

First depositor can break minting of shares #442

code423n4 opened this issue Dec 19, 2022 · 4 comments · Fixed by outdoteth/caviar#3
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working H-03 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

The attack vector and impact is the same as TOB-YEARN-003, where users may not receive shares in exchange for their deposits if the total asset amount has been manipulated through a large “donation”.

Proof of Concept

In Pair.add(), the amount of LP token minted is calculated as

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);
    }
}

An attacker can exploit using these steps

  1. Create and add 1 wei baseToken - 1 wei quoteToken to the pair. At this moment, attacker is minted 1 wei LP token because sqrt(1 * 1) = 1
  2. Transfer large amount of baseToken and quoteToken directly to the pair, such as 1e9 baseToken - 1e9 quoteToken. Since no new LP token is minted, 1 wei LP token worths 1e9 baseToken - 1e9 quoteToken.
  3. Normal users add liquidity to pool will receive 0 LP token if they add less than 1e9 token because of rounding division.
baseTokenShare = (X * 1) / 1e9;
fractionalTokenShare = (Y * 1) / 1e9;

Tools Used

Manual Review

Recommended Mitigation Steps

require(lpTokenAmount != 0, "No LP minted");
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 19, 2022
code423n4 added a commit that referenced this issue Dec 19, 2022
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 20, 2022
This was referenced Dec 20, 2022
This was referenced Dec 28, 2022
@c4-sponsor
Copy link

outdoteth marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Jan 5, 2023
@outdoteth
Copy link

Fixed in: outdoteth/caviar#3

C4-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as selected for report

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 H-03 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants