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

Pair.sol contract is susceptible to having its pricing curve (x*y = k) manipulated through a 3rd party contract calling selfdestruct() and forwarding ether. #506

Closed
code423n4 opened this issue Dec 19, 2022 · 6 comments
Labels
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 downgraded by judge Judge downgraded the risk level of this issue duplicate-383 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

If a pair is denominated in ether, a third party contract can forward ether to the contract using the selfdestruct function passing the pair's address. The impact of this is that the pair will allow its market making curve to be manipulated. Among other possibilities, this could allow the price of the fractional tokens/nft's to be artificially inflated.

Proof of Concept

The following is a common example of a contract that could forward ether to the Pair causing the contract to break its liquidity ratio:

import {Pair} from './Pair.sol';

contract AttackPair {

    Pair pair;

    constructor(Pair _pair) {
        pair = Pair(_pair);
    }

    function attack() public payable {
        address payable addr = payable(address(pair));
        selfdestruct(addr);
    }


}

Tools Used

For this audit, the tools used were slither, VS Code, and the remix ide.

Recommended Mitigation Steps

To avoid this potential price manipulation of the Pair contract, it is recommended to do the following:

  1. Declare a state variable that will track the updated balance of the contract when a payable function receives a 'msg.value'.

  2. Replace the reference to 'address(this).balance' on line 479 of the Pair.sol contract with a reference to this state variable.

@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
@Shungy
Copy link

Shungy commented Dec 19, 2022

Seems invalid.

That just gifts current liquidity providers the tokens, it does not break any invariants.

@c4-judge
Copy link
Contributor

berndartmueller marked the issue as duplicate of #353

C4-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge c4-judge removed the 3 (High Risk) Assets can be stolen/lost/compromised directly label Jan 13, 2023
@c4-judge
Copy link
Contributor

berndartmueller changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue labels Jan 13, 2023
@berndartmueller
Copy link
Member

Applying a partial credit as the submission only focuses on ETH-dominated pairs, even though the issues also exist for baseToken (contrary to the report #383)

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Jan 13, 2023
@c4-judge
Copy link
Contributor

berndartmueller marked the issue as partial-50

@C4-Staff
Copy link
Contributor

CloudEllie marked the issue as duplicate of #383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 downgraded by judge Judge downgraded the risk level of this issue duplicate-383 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)
Projects
None yet
Development

No branches or pull requests

5 participants