`FeeSplitter.curves` state variable can be replaced by a malicious contract due to lack of access controls in `setCurves` function

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L35

Vulnerability details

Impact

Due to setCurves function lacks access controls, the curves state variable can be set by anyone at any time (e.g. front-running any tx that reads from curves state variable).

    function setCurves(Curves curves_) public {
        curves = curves_;
    }

This means that a malicious actor can swap the Curves contract by one that alters the balance and/or supply for a given Curves token, which has the following consequences:

• Steal of FeeSplitter funds (ETH).
• Impair the distribution of fees amount the given Curves token.
• Halt Curves traiding.

    function balanceOf(address token, address account) public view returns (uint256) {
        return curves.curvesTokenBalance(token, account) * PRECISION;
    }

    function totalSupply(address token) public view returns (uint256) {
        //@dev: this is the amount of tokens that are not locked in the contract. The locked tokens are in the ERC20 contract
        return (curves.curvesTokenSupply(token) - curves.curvesTokenBalance(token, address(curves))) * PRECISION;
    }

The following external/public functions are compromised:

In FeeSplitter.sol (via balanceOf and totalSupply functions):

• claimFees.
• addFees.
• onBalanceChange.
• batchClaiming.

In Curves.sol (via _transferFees function, as long as there is a holders fee percentage set and feeRedistributor has been set):

• buyCurvesToken.
• buyCurvesTokenWhitelisted.
• buyCurvesTokenForPresale.
• buyCurvesTokenWithName.
• sellCurvesToken.
• sellExternalCurvesToken.

Proof of Concept

Let's imagine a few scenarios. Both have in common that the genuine Curves contract has set a a holders fee percentage (in feeEconomics.holdersFeePercent state variable) and a FeeRedistributor (in feeRedistributor state variable).

Scenario 1 - Increased claimable fees (steal of funds)

1. Mallory replaces curves with a malicious Curves contract that makes balanceOf function return a number that, after being processed by updateFeeCredit and getClaimableFees functions, will allow her to claim fees up to the FeeSplitter ETH balance.
2. Mallory calls claimFees and receives up to all FeeSplitter ETH balance as claimable fees.

    function claimFees(address token) external {
        updateFeeCredit(token, msg.sender);
        uint256 claimable = getClaimableFees(token, msg.sender); // HERE, `claimable <= address(this).balance`
        if (claimable == 0) revert NoFeesToClaim();
        tokensData[token].unclaimedFees[msg.sender] = 0;
        payable(msg.sender).transfer(claimable);
        emit FeesClaimed(token, msg.sender, claimable);
    }

Scenario 2 - Diminished claimable fees

1. Mallory replaces curves with a malicious Curves contract that makes balanceOf function return 1.
2. Alice, that has been trading Curves tokens, calls claimFees. However, the amount of ETH that receives is lower than the expected one due to owed variable (in getClaimableFees) being 0, which lowers the amout of claimable fees.

    function getClaimableFees(address token, address account) public view returns (uint256) {
        TokenData storage data = tokensData[token];
        uint256 balance = balanceOf(token, account);
        uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance; // HERE, `owed == 0`
        return (owed / PRECISION) + data.unclaimedFees[account];
    }

This case also produces inconsistencies on the amount of unclaimed fees (by claimFees function resetting the value to 0):

    function claimFees(address token) external {
        updateFeeCredit(token, msg.sender);
        uint256 claimable = getClaimableFees(token, msg.sender);
        if (claimable == 0) revert NoFeesToClaim();
        tokensData[token].unclaimedFees[msg.sender] = 0; // HERE
        payable(msg.sender).transfer(claimable);
        emit FeesClaimed(token, msg.sender, claimable);
    }

Scenario 3 - DDOS claim fees

1. Mallory replaces curves with a malicious Curves contract that makes balanceOf function return 0.
2. Alice, that has been trading Curves tokens, calls claimFees for the first time. However, her transaction reverts wtih a NoFeesToClaim (reverted by FeeRedistributor.claimFees function that couldn't make updateFeeCredit update the unclaimed fees logic and results in 0 claimable fees).

    function claimFees(address token) external {
        updateFeeCredit(token, msg.sender);
        uint256 claimable = getClaimableFees(token, msg.sender);
        if (claimable == 0) revert NoFeesToClaim(); // HERE
        tokensData[token].unclaimedFees[msg.sender] = 0;
        payable(msg.sender).transfer(claimable);
        emit FeesClaimed(token, msg.sender, claimable);
    }

Scenario 4 - DDOS subject's Curve tokens trading

1. Mallory replaces curves with a malicious Curves contract that makes totalSupply function return 0.
2. Alice, either buys or sells Curves, but her transaction reverts with a NoTokenHolders (reverted by FeeRedistributor.addFees function attempting to manage the token cummulative fees).

uint256 totalSupply_ = totalSupply(token);
if (totalSupply_ == 0) revert NoTokenHolders();

Scenario 5 - Inconsistent userTokens data

1. Mallory replaces curves with a malicious Curves contract that makes balanceOf function return 0.
2. Alice buys Curves tokens, but FeeSplitter.userTokens state variable is not properly updated to reflect that (as the condition below evaluates falsy).

    function onBalanceChange(address token, address account) public onlyManager {

        TokenData storage data = tokensData[token];
        data.userFeeOffset[account] = data.cumulativeFeePerToken;
        if (balanceOf(token, account) > 0) userTokens[account].push(token); // HERE
    }

Then getUserTokens function returns an inconsistent array of token addresses.

Tools Used

Manually reviewed.

Recommended Mitigation Steps

Add an access control modifier (e.g. onlyOwner) in the setCurves function if the current contracts architecture is kept.

    function setCurves(Curves curves_) public onlyOwner {
        curves = curves_;
    }

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4