c4-bot-5 commented 10 months ago

Lines of code

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

Vulnerability details

Impact

Whenever a user initiates a buy or sell action, the holder fees get sent to the FeeSplitter contract, invoking the onBalanceChange function to update the cumulative amounts and the user's personal reward offset. Apart from that, the array userTokens pushes the specified token if the user owns any amount of it, which leads to the array getting indefinitely increased on every buy/sell.

Proof of Concept

As it can be seen at the end of onBalanceChange: if (balanceOf(token, account) > 0) userTokens[account].push(token) if the user already has existing balance and he buys/sell only a part of it, he will push the same token to the array twice, thrice, etc. This leads to 2 things:

Off-chain reading of the user's tokens would be wrong, potentially impacting the front-end
The function getUserTokensAndClaimable would return wrong results as it could calculate the rewards for the same token multiple times, leading to another impacted off-chain monitoring, potentially fooling users into claiming earlier than expected/wished.

Tools Used

Manual Review

Recommended Mitigation Steps

Make the condition for pushing a new token more specific. If the intent is not to remove tokens on selling the entire balance, at least check if the token already is a part of the array and skip adding it if so.

Assessed type

Context

c4-pre-sort commented 10 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #104

c4-judge commented 9 months ago

alcueca changed the severity to QA (Quality Assurance)