thebrittfactor commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L263 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L180 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L68

Vulnerability details

Overview

In FeeSplitter & Curves, payments can only be made by the native token, and both contracts utilize mathematical calculations to handle ETH payments. Due to rounding errors, precision loss, and users sending excess ETH, some ETH inevitably gets stuck in both contracts.

Impact

Locked ETH remains in FeeSplitter & Curves contracts, and no recovery mechanism is available.

Proof of Concept

Various issues lead to the same issue with a common fix, consolidated for clarity:

Refund Absence: The protocol fails to refund overpayments of ETH in _buyCurvesToken(), causing funds to be permanently locked.

getPrice() Function: The Curves contract employs getPrice() to calculate prices based on supply and given amounts, leading to excess ETH in the contract due to price fluctuations.

updateFeeCredit() Function: The function responsible for calculating and updating user owed amounts, if owed < PRECISION, causes ETH to be stuck in the FeeSplitter contract.

A coded PoC shows how ETH will be stuck with simple trading:

    function testStuckEth() public {
        vm.prank(alice);
        curves.mint(alice);

        // Alice will buy the first token
        vm.prank(alice);
        curves.buyCurvesToken(alice, 1);

        // Bob buy
        uint256 amount = curves.getBuyPriceAfterFee(alice, 50);

        vm.deal(bob, amount);

        console.log("Bob ETH balance before buy :", bob.balance);

        vm.prank(bob);
        curves.buyCurvesToken{value: amount}(alice, 50);

        // Jim buy
        amount = curves.getBuyPriceAfterFee(alice, 100);

        vm.deal(jim, amount);

        console.log("Jim ETH balance before buy :", jim.balance);

        vm.prank(jim);
        curves.buyCurvesToken{value: amount}(alice, 100);

        // Bob sell
        vm.prank(bob);
        curves.sellCurvesToken(alice, 50);

        // Jim sell
        vm.prank(jim);
        curves.sellCurvesToken(alice, 100);

        console.log("Bob ETH balance after sell :", bob.balance);
        console.log("Jim ETH balance after sell :", jim.balance);
        console.log("Stuck ETH in curve         :", address(curves).balance);
    }

Logs result:

  Bob ETH balance before buy : 2816953125000000000
  Jim ETH balance before buy : 71751093750000000000
  Bob ETH balance after sell : 47376796875000000000
  Jim ETH balance after sell : 20089531250000000000
  Stuck ETH in curve         : 710171875000000000

Test Setup:

Gist link: https://gist.github.com/0xbtk/fe08c872251d66b602075915f8ae3330
Execute: forge test --mt testStuckEth -vvv

Tools Used

Manual review

Recommended Mitigation Steps

Implement functions in both FeeSplitter & Curves to allow the owner (DAO) to withdraw the locked ETH.

Assessed type

ETH-Transfer