code-423n4 / 2022-12-caviar-findings

2 stars 1 forks source link

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 closed 1 year ago

code423n4 commented 1 year ago

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.

Shungy commented 1 year ago

Seems invalid.

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

c4-judge commented 1 year ago

berndartmueller marked the issue as duplicate of #353

c4-judge commented 1 year ago

berndartmueller changed the severity to 2 (Med Risk)

berndartmueller commented 1 year ago

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 https://github.com/code-423n4/2022-12-caviar-findings/issues/383)

c4-judge commented 1 year ago

berndartmueller marked the issue as partial-50

C4-Staff commented 1 year ago

CloudEllie marked the issue as duplicate of #383