Fees will be stuck inside the `Curves` contract, or sent to the zero address

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L166-L178 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L218-L261

Vulnerability details

Fees in ETH will necessarily get stuck inside the Curves contract, whenever their percentage is not null.

This is always true for the protocolFee, when selling tokens; it can also happen for the referralFee and the holderFee, depending on the configuration, to be stuck inside the contract, or sent to the zero address.

Description

With various degrees of exposure to the configuration, there are multiple ways for fees to be stuck inside the Curves contract, or sent to the zero address. Namely, the following:

• Whenever a feesEconomics.protocolFeePercent is set:
  • it will be kept inside the contract anytime a token is sold.
• Whenever a feesEconomics.referralFeePercent is set, but there is no referral destination for the subject:
  • the referralFee will stay inside the contract anytime a token is sold.
  • if the feesEconomics.protocolFeeDestination has not been set, the referralFee will be sent to the zero address anytime a token is bought.
• Whenever a feesEconomics.holderFeePercent is set, but the feeRedistributor is the zero address, or any address that is incompatible with the FeeSplitter contract (less likely because it's set in the constructor, but it can still be incorrectly updated):
  • the holderFee will stay inside the contract anytime a token is sold.

This raises two concerns:

1. This whole mechanism is not explicit at all, as the protocol fees would be transferred when buying a token, but would be kept inside the contract when selling it. Which is at best misleading, and at worst a shady method of collecting concealed fees.

2. Even worse, some fees (the protocolFee, referralFee and even though less likely, the holderFee), depending on the conditions, will get either stuck inside the contract, or sent to the zero address. Which means that ultimately, and with an increasing gravity as the prices for subjects increase, these funds will be lost forever.

Impact

Whenever a feesEconomics.protocolFeePercent is set, the protocolFee will stay inside the contract anytime a token is sold, thus being lost forever as there is no way to withdraw ETH from the contract.

Not only this first issue cannot be avoided at all, but it is also easy to replicate a similar issue with the other fees, by missing any of the multiple required steps to set the configuration correctly.

As a result, there will necessarily be some fees getting lost, and even more as the prices increase; this can lead to a significant loss of funds, especially considering how easy it is to replicate this issue with other fees, and will reflect extremely poorly on the protocol.

Proof of Concept (short)

Take a look at the _transferFees function:

function _transferFees(address curvesTokenSubject, bool isBuy, uint256 price, uint256 amount, uint256 supply)
    internal
{
    (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee,) = getFees(price);
    {
        bool referralDefined = referralFeeDestination[curvesTokenSubject] != address(0);
        {
            // BUYING 1A. If `feesEconomics.protocolFeeDestination` was not set, this will be address(0)
            address firstDestination = isBuy ? feesEconomics.protocolFeeDestination : msg.sender;
            // BUYING 1B. If there is no referral destination for this subject, the `referralFee` will be aggregated into the `protocolFee`
            uint256 buyValue = referralDefined ? protocolFee : protocolFee + referralFee;
            // SELLING 1A. Notice how all the fees are subtracted from the price
            uint256 sellValue = price - protocolFee - subjectFee - referralFee - holderFee;
            // BUYING 1C. See here, the `referralFee` will be sent to the zero address
            (bool success1,) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
            if (!success1) revert CannotSendFunds();
        }
        {
            // SELLING 1B. `subjectFee` is always sent to the subject
            (bool success2,) = curvesTokenSubject.call{value: subjectFee}("");
            if (!success2) revert CannotSendFunds();
        }
        {
            // BUYING 2. If there is no referral destination for this subject, the `referralFee` will stay inside the contract
            // i.e. be stuck forever
            // SELLING 1C. Under the same conditions as above, it will stay inside the contract when selling as well
            (bool success3,) = referralDefined
                ? referralFeeDestination[curvesTokenSubject].call{value: referralFee}("")
                : (true, bytes(""));
            if (!success3) revert CannotSendFunds();
        }

        // BUYING 3. If `feesEconomics.holdersFeePercent` is set, but the `feeRedistributor` is incorrectly updated, the `holderFee` will stay inside the contract
        // i.e. be stuck forever here as well
        // SELLING 1D. Under the same conditions as above, it will stay inside the contract when selling as well
        if (feesEconomics.holdersFeePercent > 0 && address(feeRedistributor) != address(0)) {
            feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);
            feeRedistributor.addFees{value: holderFee}(curvesTokenSubject);
        }

        // SELLING 1E. The `protocolFee` is not even attempted to be transferred when selling tokens
    }
    emit Trade(
        msg.sender,
        curvesTokenSubject,
        isBuy,
        amount,
        price,
        protocolFee,
        subjectFee,
        isBuy ? supply + amount : supply - amount
    );
}

Proof of Concept (code)

To run the PoC with Foundry, follow the steps below:

1. Install Foundry
2. Integrate Foundry into the Hardhat project
3. Create a file called Exploit.t.sol inside test/foundry, and paste the code below

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

import {Test, console} from "forge-std/Test.sol";

import {Curves} from "../../contracts/Curves.sol";
import {CurvesERC20Factory} from "../../contracts/CurvesERC20Factory.sol";
import {FeeSplitter} from "../../contracts/FeeSplitter.sol";

uint256 constant PRECISION = 1e18;

contract Curves_exploit is Test {
    Curves public curves;

    address USER = makeAddr("user");
    address SUBJECT = makeAddr("subject");
    address PROTOCOL_DEST = makeAddr("protocol");

    function setUp() public {
        // Deploy necessary contracts and contracts required in constructor
        CurvesERC20Factory factory = new CurvesERC20Factory();
        FeeSplitter feeSplitter = new FeeSplitter();
        curves = new Curves(address(factory), address(feeSplitter));

        // Set the instance of the Curves contract in the FeeSplitter
        feeSplitter.setCurves(curves);

        // Increase the fees percentages
        uint256 subjectFeePercent = 5 * PRECISION / 100;
        uint256 referralFeePercent = 5 * PRECISION / 100;
        uint256 holdersFeePercent = 5 * PRECISION / 100;
        curves.setMaxFeePercent(subjectFeePercent + referralFeePercent + holdersFeePercent);
        curves.setExternalFeePercent(subjectFeePercent, referralFeePercent, holdersFeePercent);
    }

    function test__feesStuckOrLost_insufficientConfiguration() public {
        // Let the subject buy their first share
        vm.prank(SUBJECT);
        curves.buyCurvesToken(SUBJECT, 1);

        // Give enough funds to the user to buy a share
        uint256 price = curves.getPrice(curves.curvesTokenSupply(SUBJECT), 1);
        (,,,, uint256 fee) = curves.getFees(price);
        vm.deal(USER, price + fee);

        // Record balances
        uint256 initialUserBalance = USER.balance;
        uint256 initialSubjectBalance = SUBJECT.balance;
        uint256 initialCurvesBalance = address(curves).balance;
        uint256 initialFeeSplitterBalance = address(curves.feeRedistributor()).balance;
        uint256 initialAddressZeroBalance = address(0).balance;

        console.log("Initial balances:");
        _logBalances(false);

        // The fees were set as follows:
        // `subjectFeePercent` = 5%
        // `referralFeePercent` = 5%
        // `holdersFeePercent` = 5%
        // However, the `protocolFeePercent` is still 0%, and more importantly
        // the `protocolFeeDestination` is still the zero address.

        // Let the user buy a share from the subject
        vm.prank(USER);
        curves.buyCurvesToken{value: price + fee}(SUBJECT, 1);
        console.log("User paid", (price + fee), "wei for a share.");
        console.log("----------------------------------");

        console.log("Balances after the user bought a share:");
        _logBalances(false);

        // Right here, the address zero already received the 5% referral fee,
        // because the protocol fee destination was not yet set.
        assertGt(address(0).balance, initialAddressZeroBalance);

        // Sell the share
        vm.prank(USER);
        curves.sellCurvesToken(SUBJECT, 1);

        // Record balances
        uint256 afterSellUserBalance = USER.balance;
        uint256 afterSellSubjectBalance = SUBJECT.balance;
        uint256 afterSellCurvesBalance = address(curves).balance;
        uint256 afterSellFeeSplitterBalance = address(curves.feeRedistributor()).balance;
        uint256 afterSellAddressZeroBalance = address(0).balance;

        console.log("Balances after the user sold a share:");
        _logBalances(false);

        // ✅ As expected:
        // The subject received their 5% fee, twice (buy and sell)
        assertEq(afterSellSubjectBalance, price * 5e16 * 2 / PRECISION);
        // The fee distributor received the 5% fee for holders, twice (buy and sell)
        assertEq(afterSellFeeSplitterBalance, price * 5e16 * 2 / PRECISION);
        // The user received back their initial investment minus the fees (twice 15%)
        assertEq(afterSellUserBalance, initialUserBalance - (price * 15e16 * 2 / PRECISION));

        // 🚩 But what is unexpected is that:
        // The zero address received the 5% referral fee (buy)
        assertEq(afterSellAddressZeroBalance, initialAddressZeroBalance + (price * 5e16 / PRECISION));
        // The `Curves` contract still holds the 5% referral fee (sell)
        assertEq(afterSellCurvesBalance, initialCurvesBalance + (price * 5e16 / PRECISION));
        // ... both of which are lost forever.

        // If we had actually updated the `protocolFeeDestination` during setup,
        // the 5% referral fee would have been sent there instead of the zero address.
        // Or if the referral destination was configured for this subject,
        // the 5% referral fee would have been sent to this referree instead of the zero address.
        // (but we would still have some fees stuck in the `Curves` contract, as demonstrated below)
    }

    function test__feesStuckOrLost_withProtocolDestinationAndReferral() public {
        // We can update the protocol fee percentage and destination to figure out the difference
        (,,,,, uint256 maxFeePercent) = curves.feesEconomics();
        curves.setMaxFeePercent(maxFeePercent + 5e16);
        curves.setProtocolFeePercent(5e16, PROTOCOL_DEST);
        // And even set a referral destination for this subject (their own address)
        vm.prank(SUBJECT);
        curves.setReferralFeeDestination(SUBJECT, SUBJECT);

        // Let the subject buy their first share
        vm.prank(SUBJECT);
        curves.buyCurvesToken(SUBJECT, 1);

        // Give enough funds to the user to buy a share
        uint256 price = curves.getPrice(curves.curvesTokenSupply(SUBJECT), 1);
        (,,,, uint256 fee) = curves.getFees(price);
        vm.deal(USER, price + fee);

        // Record balances
        uint256 initialUserBalance = USER.balance;
        uint256 initialSubjectBalance = SUBJECT.balance;
        uint256 initialCurvesBalance = address(curves).balance;
        uint256 initialFeeSplitterBalance = address(curves.feeRedistributor()).balance;
        uint256 initialProtocolDestBalance = PROTOCOL_DEST.balance;

        console.log("Initial balances:");
        _logBalances(true);

        // The fees were set as follows:
        // `subjectFeePercent` = 5%
        // `referralFeePercent` = 5%
        // `holdersFeePercent` = 5%
        // `protocolFeePercent` = 5%
        // `protocolFeeDestination` = PROTOCOL_DEST

        // Let the user buy a share from the subject
        vm.prank(USER);
        curves.buyCurvesToken{value: price + fee}(SUBJECT, 1);
        console.log("User paid", (price + fee), "wei for a share.");
        console.log("----------------------------------");

        console.log("Balances after the user bought a share:");
        _logBalances(true);

        // This time there are no funds sent to the zero address,
        assertEq(address(0).balance, 0);

        // Sell the share
        vm.prank(USER);
        curves.sellCurvesToken(SUBJECT, 1);

        // Record balances
        uint256 afterSellUserBalance = USER.balance;
        uint256 afterSellSubjectBalance = SUBJECT.balance;
        uint256 afterSellCurvesBalance = address(curves).balance;
        uint256 afterSellFeeSplitterBalance = address(curves.feeRedistributor()).balance;
        uint256 afterSellProtocolDestBalance = PROTOCOL_DEST.balance;

        console.log("Balances after the user sold a share:");
        _logBalances(true);

        // ✅ As expected:
        // The subject received their 5% subject fee, twice (buy and sell) + 5% referral fee, twice (buy and sell)
        assertEq(afterSellSubjectBalance, price * 10e16 * 2 / PRECISION);
        // The fee distributor received the 5% fee for holders, twice (buy and sell)
        assertEq(afterSellFeeSplitterBalance, price * 5e16 * 2 / PRECISION);
        // The user received back their initial investment minus the fees (twice 20%)
        assertEq(afterSellUserBalance, initialUserBalance - (price * 20e16 * 2 / PRECISION));

        // 🚩 But what is unexpected is that:
        // The protocol destination received only the 5% protocol fee (buy); not on sell
        assertEq(afterSellProtocolDestBalance, initialProtocolDestBalance + (price * 5e16 / PRECISION));
        // The `Curves` contract still holds the 5% protocol fee (sell)
        assertEq(afterSellCurvesBalance, initialCurvesBalance + (price * 5e16 / PRECISION));
        // ... the latter being, once again, unable to be withdrawn.
    }

    function _logBalances(bool withProtocolDest) internal {
        console.log("User:", USER.balance, "wei");
        console.log("Subject:", SUBJECT.balance, "wei");
        console.log("Curves:", address(curves).balance, "wei");
        console.log("FeeSplitter:", address(curves.feeRedistributor()).balance, "wei");
        if (withProtocolDest) {
            console.log("Protocol destination:", PROTOCOL_DEST.balance, "wei");
        } else {
            console.log("Address zero:", address(0).balance, "wei");
        }
        console.log("----------------------------------");
    }
}

4. Run forge test --mc Curves_exploit

Mitigation

There are multiple options to mitigate this issue, depending on the degree of control/simplicity.

1. Funds recovery

A very basic first step, might be to add a function allowing the owner to withdraw additional funds from the Curves contract; meaning, funds that are not allocated to current holders. This would allow recovering any funds mistakenly sent to the contract, e.g. due to a wrong configuration in a UI, or miscalculations in the fees distribution.

Basically, using a variable to keep track of expected funds (increase on buy, decrease on sell), and a privileged function to withdraw any surplus (the difference between the expected and actual funds).

However, this should not be the only step in solving this issue, as it would practically mean charging hidden fees (e.g. protocolFee) from the users; a very shady practice, which would not be in the best interest of the protocol.

2. Fees distribution

Something that could help, would be to add some explicit checks in the _transferFees function, for destination addresses. On how to handle different fees, based on each other (e.g. the referralFee when there is no destination), this is purely a design choice, on which I cannot comment. Basically, you need to handle the following cases:

• When selling tokens:
  • deciding what to do with the protocolFee (currently not handled at all).
• When buying tokens:
  • defining firstDestination when feesEconomics.protocolFeeDestination is not set (== address(0));
  • depending on the answer above, keep in mind that currently, if there is no referral destination, the referralFee will be sent along the protocolFee.
• Both when buying and selling tokens:
  • handling the referralFee when there is no referralFeeDestination for the subject;
  • handling the holderFee when there is no feeRedistributor set, or it is incompatible with the FeeSplitter contract.

Most of this can be done by performing checks on the fees configuration before calculating the values of various fees. A proposition to mitigate these could be to update the functions as follows:

// Handles the `referralFee` when the destination is not set
// Handles the `holderFee` when the `feeRedistributor` is not set (or not compatible)
function getFees(address curvesTokenSubject, uint256 price)
    public
    view
    returns (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holdersFee, uint256 totalFee)
{
    // Only evaluate the protocol fee if there is a destination (otherwise it's 0)
    if (feesEconomics.protocolFeeDestination != address(0)) {
        protocolFee = (price * feesEconomics.protocolFeePercent) / 1 ether;
    }
    subjectFee = (price * feesEconomics.subjectFeePercent) / 1 ether;
    // Only evaluate the referral fee if there is a destination for this subject
    if (referralFeeDestination[curvesTokenSubject] != address(0)) {
        referralFee = (price * feesEconomics.referralFeePercent) / 1 ether;
    }
    // Only evaluate the holders fee if there is the feeRedistributor contract is set
    // (by checking the selector of the `onBalanceChange` function; `addFees` could work as well)
    if (feeRedistributor.onBalanceChange.selector == bytes4(keccak256("onBalanceChange(address,address)"))) {
        holdersFee = (price * feesEconomics.holdersFeePercent) / 1 ether;
    }

    totalFee = protocolFee + subjectFee + referralFee + holdersFee;
}

...

function _transferFees(address curvesTokenSubject, bool isBuy, uint256 price, uint256 amount, uint256 supply)
    internal
{
    (uint256 protocolFee, uint256 subjectFee, uint256 referralFee, uint256 holderFee,) =
        getFees(curvesTokenSubject, 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;
            // If `firstDestination` is the zero address => `feesEconomics.protocolFeeDestination` is the zero address
            // => `feesEconomics.protocolFeePercent` is 0 => `protocolFee` is 0 as well
            // In this case, if `referralDefined` is false => `referralFee` is 0 => `buyValue` is 0
            // So `buyValue` will be 0 whenever `firstDestination` is the zero address, meaning that we can skip this call
            if (firstDestination != address(0)) {
                (bool success1,) = firstDestination.call{value: isBuy ? buyValue : sellValue}("");
                if (!success1) revert CannotSendFunds();
            }
        }

        // Transfer the `protocolFee` on selling as well
        // (feel free to replace `success1`, `success2` considering this new call)
        if (!isBuy && feesEconomics.protocolFeeDestination != address(0)) {
            (bool success1,) = feesEconomics.protocolFeeDestination.call{value: protocolFee}("");
            if (!success1) revert CannotSendFunds();
        }

        ...
    }

    ...
}

Assessed type

ETH-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #28

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b