Unrestricted claiming of fees due to missing balance updates in `FeeSplitter`

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

The FeeSplitter contract is designed to distribute fees among token holders. It employs an accumulator pattern to distribute rewards over time among users who do not sell or withdraw their tokens as ERC20s. This pattern works by maintaining an accumulator representing the cumulative total of the reward rate over time, and updating it for each user every time their balance changes.

However, the current implementation does not update the accumulator associated with each user during token transfers, deposits, or withdrawals. The onBalanceChange() function, which is responsible for updating the accumulator following changes in a user's balance, is exclusively called from Curves._transferFees(), which is only called during buy and sell transactions.

This oversight can be easily exploited to drain the FeeSplitter contract of its fees. An attacker could repeatedly transfer the same tokens to new accounts and claim fees every time. This is possible because data.userFeeOffset[account] will be zero for every new account they transfer to, while the claimable rewards are calculated using the current balance returned by the Curves contract.

Since there is no limit to the amount of rewards that may accumulate in the FeeSplitter contract, this can be considered loss of matured yield and is hence classified as high severity.

Proof of Concept

Consider the following scenario:

1. Alice buys any amount of curve tokens.
2. Alice transfers her tokens to a new account, Account1, by calling transferCurvesToken().
3. The onBalanceChange() function is not triggered during the transfer, so data.userFeeOffset[account] for Account1 is not updated.

4. Account1 can now call FeeSplitter.claimFees() and will receive fees as if it had been holding the tokens since the beginning, as data.userFeeOffset[account] for this account is 0. https://github.com/code-423n4/2024-01-curves/blob/516aedb7b9a8d341d0d2666c23780d2bd8a9a600/contracts/FeeSplitter.sol#L63-L88

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

5. Alice creates another new account, Account2, and transfers the tokens from Account1 to Account2.
6. The onBalanceChange(token, account) function is not triggered during this transfer, so data.userFeeOffset[account] for Account2 is zero.
7. Account2 can now claim fees.
8. Alice can repeat this process, creating new accounts and transferring tokens between them, to drain the contract of its fees.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, the onBalanceChange(token, account) function should be triggered during all token transfers, deposits, and withdrawals. For token transfers, it should be triggered for both accounts. This will ensure that userFeeOffset is accurately tracked, preventing users from claiming fees multiple times.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #222

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as duplicate of #1491

[raymondfam]

The root cause is due to not calling updateFeeCredit() diligently. Deduping it to #1491 to be more appropriately categorized.

[c4-judge]

alcueca marked the issue as satisfactory

[0xEVom]

@alcueca I consider this to be the root cause of both #874 and #1491 (missing balance updates):

However, the current implementation does not update the accumulator associated with each user during token transfers, deposits, or withdrawals

Would you consider selecting this for report, and collecting all of the current duplicates of both findings under this one?

[c4-judge]

alcueca marked the issue as selected for report

[c4-judge]

alcueca marked the issue as primary issue

[alcueca]

You know, I actually agree with you. I like that this report takes a global aproach at the issue.

[0xEVom]

@alcueca @thebrittfactor quick heads up that #874 is still a separate issue. I think that was an oversight and @alcueca wanted to make it a duplicate of this one.

[c4-sponsor]

andresaiello (sponsor) confirmed