Covalent contest - gpersoon's results

Handle

gpersoon

Vulnerability details

Impact

The function depositRewardTokens divides the "amount" of tokens by allocatedTokensPerEpoch to calculate the endEpoch. When "amount" isn't a multiple of allocatedTokensPerEpoch the result of the division will be rounded down, effectively losing a number of tokens for the rewards.

For example if allocatedTokensPerEpoch is set to 3e18 and "amount" is 100e18 then endEpoch will be increased with 33e18 and the last 1e18 tokens are lost.

A similar problem occurs here:

• in setAllocatedTokensPerEpoch(), with the recalculation of endEpoch
• in takeOutRewardTokens(), with the retrieval of tokens
• in _stake(), when initializing endEpoch (e.g. when endEpoch==0)

Proof of Concept

https://github.com/code-423n4/2021-10-covalent/blob/ded3aeb2476da553e8bb1fe43358b73334434737/contracts/DelegatedStaking.sol#L90-L98

https://github.com/code-423n4/2021-10-covalent/blob/ded3aeb2476da553e8bb1fe43358b73334434737/contracts/DelegatedStaking.sol#L368-L383

Tools Used

Recommended Mitigation Steps

In depositRewardTokens() add, in the beginning of function, before the if statement: require(amount % allocatedTokensPerEpoch == 0,"Not multiple");

In takeOutRewardTokens() add: require(amount % allocatedTokensPerEpoch == 0,"Not multiple");

Update setAllocatedTokensPerEpoch() to something like:

if (endEpoch != 0) { ... uint128 futureRewards = ... require(futureRewards % amount ==0,"Not multiple"); ...
} else { // to prevent issues with _stake() require(rewardsLocked % allocatedTokensPerEpoch==0,"Not multiple"); }