c4-bot-1 commented 9 months ago

Lines of code

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

Vulnerability details

The addFees function allows managers to add fees to the contract, but does not automatically trigger a distribution of these fees to token holders.

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

Mitigation

To ensure that fees are distributed to token holders, you can introduce a mechanism that updates each token holder's unclaimedFees whenever new fees are added.

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

    for (uint256 i = 0; i < holders.length; i++) {
        updateFeeCredit(token, holders[i]);
    }
}

Impact

With the current approach, fees added to the contract could remain unclaimed, leading to a mismatch between the fees that token holders expect to receive and what is actually claimable. Token holders would need to actively claim their fees or rely on the contract manager to trigger an update, which could lead to delays or missed fee distributions.

Mitigation Explanation:

The mitigation code introduces a new function addFeesAndDistribute that takes an additional parameter: an array of token holder addresses.
After adding the new fees to the cumulativeFeePerToken, the function iterates over the list of token holders and calls updateFeeCredit for each one, ensuring that their unclaimedFees are updated accordingly.
This ensures that as soon as fees are added, they are also distributed to the token holders, reflecting the new balances immediately.
The contract manager is responsible for providing the list of current token holders, which could be obtained from an off-chain process or a separate contract that tracks token ownership.
This approach ensures that fee distributions are proactive and token holders do not need to take any action to receive their share of the fees.

Assessed type

Context

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as primary issue

raymondfam commented 9 months ago

Low QA for failing to elaborate on fee loss due to not updating users' unclaimed fees before having data.userFeeOffset[account] assigned the latest value of data.cumulativeFeePerToken.