Holder fee accounting is not updated on holder balance change; FeeSplitter can be drained

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

When tokens are deposited or withdrawn (converted between Curves and ERC20) appropriate actions are not taken to update the holder fee accounting, such that unclaimed fees are lost on withdrawn tokens and (contrary to design) holder fees can be claimed on exported/withdrawn tokens by depositing them immediately before claiming fees on them. The amount that can be claimed is the amount due to each currently deposited token when fees are added. Since this implies that more fees can be claimed than are available, this can be leveraged to drain the FeeSplitter contract.

Proof of Concept

For a given token, fees enter FeeSplitter through

function addFees(address token) public payable onlyManager {
    uint256 totalSupply_ = totalSupply(token);
    if (totalSupply_ == 0) revert NoTokenHolders();
    TokenData storage data = tokensData[token];
    data.cumulativeFeePerToken += (msg.value * PRECISION) / totalSupply_;
}

which distributes the added fees equally over the totalSupply_ of the token. This only counts the currently held tokens (not exported/withdrawn as ERC20s):

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;
}

Fees are then claimed by calling

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;
    payable(msg.sender).transfer(claimable);
    emit FeesClaimed(token, msg.sender, claimable);
}

The functions in the first two lines

function updateFeeCredit(address token, address account) internal {
    TokenData storage data = tokensData[token];
    uint256 balance = balanceOf(token, account);
    if (balance > 0) {
        uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
        data.unclaimedFees[account] += owed / PRECISION;
        data.userFeeOffset[account] = data.cumulativeFeePerToken;
    }
}

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;
    return (owed / PRECISION) + data.unclaimedFees[account];
}

somewhat redundantly updates and returns the amount of unclaimed fees as the unclaimed fees per token times the user's balance of tokens, which again is the balance of only the held tokens:

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

(curvesTokenBalance is updated in Curves._transfer() to reflect this.)

The issue is that this balance can change, without changing data.cumulativeFeePerToken or data.userFeeOffset[account]. The latter are only updated when claiming fees, as seen above (or similarly in batchClaiming()), and in onBalanceChange() which is only called in Curves._transferFees() which is executed only when buying or selling Curves tokens. balance, on the other hand, changes precisely when tokens are deposited or withdrawn.

Suppose fees are added. Each held token, totalSupply_, which is the sum of all users' current balances, receives an equal share. If the user then claimFees() he will thus get (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance, i.e. he will get the number balance of these added shares. So if he deposits tokens, thus decreasing his balance, and then claims the fees he will lose some of these shares, since claiming fees resets claimable fees to zero, and if he withdraws fees, thus increasing his balance, and then claims the fees he will be awarded more shares than he had rights to when the fees where added.

The last case implies that a user can keep all his tokens in the ERC20 form, but still claim holder fees on them by just momentarily redepositing them whenever he claims fees. Note that this is not just a change in functionality, but an overstatement; the user is claiming these fees at the expense of others, for whom there will not be enough fair shares left to claim.

Furthermore, this can be leveraged in a direct attack to drain the FeeSplitter contract. Suppose total fees are 10 %, and the holder fee is 2 %. The attacker creates his own token and buys (at least) 7 tokens, and withdraws all but one (otherwise fees cannot be added). At this stage, if he buys and sells a token then 3 % of the price is distributed to the one held token (a half on buy and the full fee on sell). The cost of this is not greater than 20 % of the price. So by redepositing the 6 tokens he can now claim 7 * 3 % = 21 % of the price, for a profit of 1 % of the price. Withdraw the 6 tokens and repeat until the FeeSplitter is drained.

Recommended Mitigation Steps

When balance changes for an account, adjust data.userFeeOffset[account]) such that (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance remains constant (doesn't increase), or set data.userFeeOffset[account] = data.cumulativeFeePerToken and move the fees over to data.unclaimedFees[account].

Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #222

[raymondfam]

The root cause is due to not calling updateFeeCredit() when calling withdraw() and deposit().

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca marked the issue as satisfactory

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as duplicate of #247