Open code423n4 opened 3 years ago
kitti-katy
commented
3 years ago Agreed, the original assumption was that the owner would always make sure the take out and deposit amount is multiple of emission rate. But yes, this is good to add the check. Also it is not that risky since the emission rate wouldn't be that high per epoch and the loss will always be less than the emission rate.
GalloDaSballo
commented
3 years ago Agree with the finding, since it's a rounding error the max loss in rewards can at most be 1 less than the denominator
That said, this is a Medium Severity Finding as per the doc:
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
Where in this case the rounding is a way to leak value (loss of yield)
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:
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"); }