onBalanceChange adds tokens to userTokens without checking if they already exist

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

onBalanceChange pushes to an array every time a users token balance changes to a non zero value. This will add tokens which already exist in the userTokens array and become excessively long.

Proof of Concept

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Check if the token already exists in the array before adding it.

Assessed type

Other

[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 marked the issue as selected for report

[c4-judge]

alcueca changed the severity to 3 (High Risk)

[c4-judge]

alcueca changed the severity to 2 (Med Risk)

[alcueca]

Since the bug only affects a view function, not called by any transactional function in the contract, the severity is QA. There is no DoS in any contract in scope, as the function can be called externally regardless of gas use.

[c4-judge]

alcueca changed the severity to QA (Quality Assurance)

[c4-judge]

alcueca marked the issue as grade-b

[c4-judge]

alcueca marked the issue as not selected for report