Unbounded userTokens growth

[c4-bot-3]

Lines of code

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

Vulnerability details

Impact

In FeeSplitter, function onBalanceChange updates the array of user's tokens if the user's balance is non-zero. However, due to not checking if the token is already present nor a way to remove tokens, the array will keep growing indefinitely. For example, as it is used by getUserTokensAndClaimable, it will iterate through both repeated and zero-balance tokens, and depending on the size either it will always revert or will require users to pay a huge amount of gas.

Proof of concept

Arrays in Solidity are not sets, which means you can have repeated elements. If we go to the definition of userTokens in:

FeeSplitter, line 29

    mapping(address => address[]) internal userTokens;

we see it maps user addresses with an array of associated tokens. Now, if we go to:

FeeSplitter, onBalancechange

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

we see that if the user's token balance is non-zero, it will append the token address to its associated userToken array. Such a function is called by:

• Curves::_buyCurvesToken -> Curves:: _transferFee -> FeeSplitter::onBalanceChange (as well as its public functions)
• Curves::sellCurvesToken -> Curves:: _transferFee -> FeeSplitter::onBalanceChange

As users can buy multiple times the same curvesTokenSubject, as well as selling them in little batches, which makes its balance non-zero in those situations, it will keep pushing the same curvesTokenSubject to userTokens again and again in every buy/sell operation. Because of that, the next situations arise:

1. Worst case scenario $\Rightarrow$ users won't be able to call getUserTokens, getUserTokensAndClaimable nor batchClaiming due to memory expansion eating all the transaction gas, triggering a return bomb
2. Normal scenario $\Rightarrow$ user's will be forced to traverse linearly userTokens when calling getUserTokensAndClaimable, paying a huge amount of gas as it will loop through correct tokens, repeated ones as well as those the user sold, that is, its balance is zero but they remain there as there is no way to remove them.

Recommended Mitigation Steps

Use OZ EnumerableSet instead of a vanilla Solidity array and implement a "remover" so that users can remove tokens whose balance is $0$ (for example, after selling all the associated curves token).

Assessed type

Loop

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #684

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b