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

Possible loss of funds when adding liquidity #30

Closed
code423n4 opened this issue Dec 13, 2022 · 3 comments
Closed

Possible loss of funds when adding liquidity #30

code423n4 opened this issue Dec 13, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-376 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

Lines of code

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

Vulnerability details

Possible loss of funds when adding liquidity

Summary

The Pair contract does not return excess funds to the user when adding liquidity through add() or nftAdd() which can result in loss of funds through external bugs.

Detailed description

Pair.add() calls into Pair.addQuote to calculate how much LP tokens to mint given the input of base tokens and fractional tokens (fTokens). The share of both inputs is calculated and the lower share is used to calculate the LP token amount through Math.min(baseTokenShare, fractionalTokenShare).
Through an external bug, such as an UI bug or an error in the integration of Caviar in a third-party service, users could overpay by an infinite amount by supplying more of one of the two assets than would be neccessary. These excess funds would get split evenly across all LP token holders instead of being returned to the user.

Affected functions:

  • add()
  • nftAdd() since it calls into add()

Recommended mitigation

Calculate excess funds when adding liquidity and return them to the user.

PoC

Paste the following unit test into the test/Pair/unit directory and run it with forge test -vv -m testOverpay. The -vv flag is important in order to see the console.log output.

The PoC demonstrates a user overpaying and losing 4499 ETH.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "../../shared/Fixture.t.sol";

contract OverpayTest is Fixture {
    function setUp() public {
        // Setup ETH and fToken balance
        deal(address(this), 10_000 ether);
        deal(address(ethPair), address(this), 100 ether, true);

        // Setup the pool with 1 ETH and 50 fToken
        // This state is considered to have been created by other third-parties.
        // They also own the LP token, that's why it's sent out below.
        ethPair.add{value: 1 ether}(1 ether, 50 ether, 1 ether);

        // Send out the LP tokens
        ethPairLpToken.transfer(
            address(1),
            ethPairLpToken.balanceOf(address(this))
        );
    }

    function testOverpay() public {
        // Save balances before add & remove
        uint256 ethBefore = address(this).balance;
        uint256 tokensBefore = ethPair.balanceOf(address(this));

        // Add liquidity while overpaying significantly in ETH
        ethPair.add{value: 9_000 ether}(9_000 ether, 50 ether, 1 ether);

        // Remove liquidity to see how much we get back
        ethPair.remove(ethPairLpToken.balanceOf(address(this)), 0, 0);

        // Get balances and calculate diff
        uint256 ethAfter = address(this).balance;
        uint256 tokensAfter = ethPair.balanceOf(address(this));
        int256 ethDiff = int256(ethAfter) - int256(ethBefore);
        int256 tokensDiff = int256(tokensAfter) - int256(tokensBefore);

        console.log("ETH diff:");
        console.logInt(ethDiff / 1 ether);
        console.log("fToken diff:");
        console.logInt(tokensDiff / 1 ether);
    }
}
@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 13, 2022
code423n4 added a commit that referenced this issue Dec 13, 2022
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as duplicate of #376

@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 and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 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-376 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