Risk of not settling userFeeOffset status when transferring curvesTokenSubject

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L296-L325 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L75 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L40

Vulnerability details

Impact

In the protocol, when a user purchases a curvesTokenSubject, they can claim allocated fees via the FeeSplitter contract's claimFees function. The claimable fees are calculated as:

(data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance

Where balance is the user's curvesTokenSubject balance, cumulativeFeePerToken is the protocol's current accumulated fees, and userFeeOffset[account] is the accumulated fees the user is entitled to.

In theory, userFeeOffset[account] should be updated when the user's curvesTokenSubject balance changes, and settle the fees the user is entitled to for holding the curvesTokenSubject.

Unfortunately, when users transfer curvesTokenBalance via transferCurvesToken or transferAllCurvesTokens, the current fees for the from address are not settled. This results in reduced claimable fees for the from address. More severely, the to address can claim much more fees than expected. An attacker can exploit this to drain all fees in the FeeSplitter contract.

Proof of Concept

In the FeeSplitter contract, users can claim fees via the claimFees function. https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L80-L87

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

It will first update the user's unclaimedFees and userFeeOffset via the updateFeeCredit function, where unclaimedFees is the amount of fees the user can claim. https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L67-L68

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

unclaimedFees is calculated as: (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance

This calculation relies on the user's curvesTokenSubject balance, userFeeOffset[account], and cumulativeFeePerToken. cumulativeFeePerToken and userFeeOffset[account] are only updated when the user buys/sells curvesTokenSubjects. After update, userFeeOffset[account] will equal cumulativeFeePerToken. This avoids the user claiming fees immediately after buying, and claiming fees after selling. https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L247-L248 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L274 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L292 https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L89-L100

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

function _buyCurvesToken(address curvesTokenSubject, uint256 amount) internal {
    ...
    _transferFees(curvesTokenSubject, true, price, amount, supply);
    ...
}

function sellCurvesToken(address curvesTokenSubject, uint256 amount) public {
    ...
    _transferFees(curvesTokenSubject, false, price, amount, supply);
}

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

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

However, a user's balance changes not only on buying/selling of curvesTokenSubjects, but also when transferring via transferCurvesToken/transferAllCurvesTokens. https://github.com/code-423n4/2024-01-curves/blob/main/contracts/Curves.sol#L296-L325

function transferCurvesToken(address curvesTokenSubject, address to, uint256 amount) external {
    if (to == address(this)) revert ContractCannotReceiveTransfer();
    _transfer(curvesTokenSubject, msg.sender, to, amount);
}

// Transfer the total balance of all my tokens to another address. Can be used for migrating tokens.
function transferAllCurvesTokens(address to) external {
    if (to == address(this)) revert ContractCannotReceiveTransfer();
    address[] storage subjects = ownedCurvesTokenSubjects[msg.sender];
    for (uint256 i = 0; i < subjects.length; i++) {
        uint256 amount = curvesTokenBalance[subjects[i]][msg.sender];
        if (amount > 0) {
            _transfer(subjects[i], msg.sender, to, amount);
        }
    }
}

function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
    ...
    curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
    curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;
    ...
}

Unfortunately, Curves does not call FeeSplitter's updateFeeCredit to settle the user's fees before curvesTokenSubject transfers. This allows malicious users to continually transfer then claim fees to profit more. Here's a simple example:

1. Assume from address has 1 curvesTokenSubject, to has 0, FeeSplitter's cumulativeFeePerToken is 100.
2. from transfers 1 curvesTokenSubject to to, now to's balance is 1.
3. to claims fees, since its userFeeOffset is unupdated, it's 0. cumulativeFeePerToken is current accumulated fees, so data.cumulativeFeePerToken - data.userFeeOffset[account] is 100. to's balance is 1. Therefore (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance is 100.
4. So to can claim 100 fees without paying anything.

In actual exploits, if the FeeSplitter contract has insufficient balance to pay the fees, the attacker can use a flashloan to cover the difference and drain all fees from the FeeSplitter contract in one transaction.

Tools Used

N/A

Recommended Mitigation Steps

It is recommended for the FeeSplitter contract to provide an updateFeeCredit function that can be called by the Curves contract. Before a user's curvesTokenSubject is transferred, their fees should be settled via updateFeeCredit. Consider the following fixes:

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

function _transfer(address curvesTokenSubject, address from, address to, uint256 amount) internal {
    if (amount > curvesTokenBalance[curvesTokenSubject][from]) revert InsufficientBalance();

    // If transferring from oneself, skip adding to the list
    if (from != to) {
        _addOwnedCurvesTokenSubject(to, curvesTokenSubject);
    }

    feeRedistributor.updateFeeCredit(curvesTokenSubject, from);
    feeRedistributor.updateFeeCredit(curvesTokenSubject, to);

    curvesTokenBalance[curvesTokenSubject][from] = curvesTokenBalance[curvesTokenSubject][from] - amount;
    curvesTokenBalance[curvesTokenSubject][to] = curvesTokenBalance[curvesTokenSubject][to] + amount;

    emit Transfer(curvesTokenSubject, from, to, amount);
}

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

function updateFeeCredit(address token, address account) external onlyManager {
    _updateFeeCredit(token, account);
}

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

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #41

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca marked the issue as satisfactory