Core Contracts

Summary

At FeeCollector users will only be able to claim fees once because their claimed amounts is tracked with total amounts distributed.

Vulnerability Details

Whenever a user does FeeCollector::claimRewards(), the following happens here:

An user-wise state is saved with a totalDistributed data which is meant to represent all distributed for all users. This ends up being problematic.

Impact

User will only be able to claim rewards once most likely.

Prof Of Concept

This is due to the final check on the _calculatePendingRewards() func, called here.

This check usage is to prevent a user from claiming already claimed rewards:

But as seen earlier, the amount saved on userRewrads is the total. So share will probably be smaller, here are numerical examples:

• First distribution:

  • Distribute 100 tokens.

  • User share is 10% = 10 tokens.

  • User claims 10 tokens. Then userClaim[user] = totalDistributed = 100.

• Second distribution (see totalDistributed is a ever growing variable, only increased with rewards distribution here):

  • Distribute 100 tokens.

  • User share is 10% = 10 tokens.

  • User tries to claim 10 tokens. But the return check does:

    • share = (200 * 10) / 100 = 20.

    • 20 > 100 == false -> return 0.

The user won't be able to claim rewards. Note that for a user with 10% of the voting power, the amount of rewards that would have to be distributed should be more than x10. In this case > 1000 tokens, then 10% of 1001 is 100.1 tokens and the return would return 0.1 tokens.

⚠️ Note: Furthermore, the rewards are wrongly calculated but I've submited it as a separate report as it has different fixes and root caue. Even if calculations were correct this issue would still be present.

Recommendations

Instead of writing the totalDistribued to userRewards[msg.sender] here, it should be the pendingReward amount that is actually transferred.