Core Contracts

Summary

When a user is eligible for claiming rewards within the protocol, specifically within the FeeCollector.sol contract, we allow them to execute the _claimRewards function. This would return a bonus to the user in respect of how many tokens they are owed for their different protocol activities. However, the user's mapping state is incorrectly updated therefore resulting in a permanent DoS regarding the reward claiming functionality.

Vulnerability Details

Before the protocol distributes the rewards, it calculates them via _calculatePendingRewards within the initial call of claimRewards. Then, within _calculatePendingRewards, we calculate the users total voting power via getVotingPower(). All of these operations within this context as so far, correct.

However, the result from this call is saved into pendingReward. This is fine, but when we allocate the rewards to the users mapping userRewards[user], we invoke the variable of totalDistributed.

This operation is flawed, because we are directly setting the user's rewards to the value of totalDistributed, which is responsible for tracking tokens which are "distributed historically" (i.e. throughout the history of the contract).

The reason this is problematic, is because on the 2nd function call in the future regarding claimRewards, and more specifically, _calculatePendingRewards, we will not meet the return condition where we do the following operation:

return share > userRewards[user] ? share - userRewards[user] : 0;

Ultimately, upon first time claiming rewards this is OK, but on the second time, the user's mapping of userRewards is set to the totalDistributed amount, and therefore the calculated share value in respect of the user's total voting power:

uint256 share = (totalDistributed * userVotingPower) / totalVotingPower

will never suffice for a valid return of their owed rewards, as they are permanently going to be higher than the share value.

Chisel PoC:

Additionally, the return statement also penalises users who do not choose to claim rewards often (for instance, 1 year). Because the code does not invoke a state change of the mapping when rewards are claimed, this would mean that if the userRewards[user] is populated, then there could be a scenario where the calculated share (reward return) is higher than the mapping of the user's userRewards. Therefore, if it is, then we deduct the user's unclaimed rewards from the share that we are about to return to them, therefore not allowing them to claim their full owed amount.

Impact

Users can only claim once, and then never again unless there is a direct reset of their user mapping. Or, in other scenarios, are unable to retrieve the full owed reward amount.

Tools Used

Chisel

Recommendations

The protocol should change: userRewards[user] = totalDistributed; to userRewards[user] = 0; in order to successfully account for the user's claimed rewards once successful.