Yield micro contest #1 - moose-code's results

Handle

moose-code

Vulnerability details

Impact

rewardsPerToken_.accumulated can stay constant while rewardsPerToken_.lastUpdated is continually updated, leading to no actual rewards being distributed. I.e. No rewards accumulate.

Proof of Concept

Line 115, rewardsPerToken_.accumulated could stay constant if there are very quick update intervals, a relatively low rewardsPerToken_.rate and a decent supply of the ERC20 token.

I.e. imagine the token supply is 1 billion tokens (quite a common amount, note even if a supply of only say 1 million tokens this is still relevant). i.e. 1e27 wei.

Line 115 has 1e18 * timeSinceLastUpdated * rewardsPerToken_.rate / _totalSupply

timeSinceLastUpdated can be crafted to be arbitrarily small by simply transferring or burning tokens, so lets exclude this term (it could be 10 seconds etc). Imagine total supply is 1e27 as mentioned.

therefore, 1e18 * rewardsPerToken_.rate / 1e27, which shows that if the rewardsPerToken_.rate is < 1e9, something which is very likely, then the accumulated amount won't increment, as there are no decimals in solidity and this line of code will evaluate to adding zero. While this is rounded down to zero, critically, rewardsPerToken_.lastUpdated = end; is updated.

The reason I have labelled this as a high risk is the express purpose of this contract is to reward users with tokens, yet a user could potentially quite easily exploit this line to ensure no one ever gets rewards and the accumulated amount never increases.

Given a fairly large token supply, and a relatively low emissions rate is set, that satisfies the above equation, for the entire duration of the rewards period, the user simply sends tokens back and forth every couple seconds (gas limitations, but layer 2), to keep the delta timeSinceLastUpdated close to 1.

This way the accumulated amount will never tick up, but time keeps being counted.

Furthermore, I would say this is high risk as this wouldn't even need an attacker. Given the transfer function is likely often being called by users, timeSinceLastUpdated will naturally be very low anyways.

Even if not so extreme as the above case, Alberto points out that "rounding can eat into the rewards" which is likely to be prevalent in the current scenario and make a big impact over time on the targeted vs actual distribution.

Again this problem is more likely to occur in naturally liquid tokens where lots of transfer, mint or burn events occur.

Tools Used

Manual analysis.

Recommended Mitigation Steps

As suggested by Alberto, the simplest it to probably not update the rewardsPerToken_.lastUpdated field if rewardsPerToken_.accumulated does not change. Although this change should be closely scrutinized to see it doesn't introduce bugs elsewhere.

Handle

moose-code

Vulnerability details

Impact

Users have essentially have an option to either claim currently earned reward amounts on future rewards tokens, or the current rewards token.

Although stated on line 84, it does not take into account the implications and lock in this contract will have on the future value of new tokens able to be issued via rewards.

Proof of Concept

Smart users will monitor the mempool for setRewards transactions. If the new reward token (token b) is less valuable than the old reward token (token a), they front run this transaction by calling claim. Otherwise they let their accrued 'token a' roll into rewards of of the more valuable 'token b'.

Given loads of users will likely hold these tokens from day 1, there will potentially be thousands of different addresses squatting on rewards.

Economically, given the above it makes sense that the value of new reward tokens, i.e. 'token b' should always be less than that of 'token a'. This is undesirable in a rewards token contract as there is no reliable way to start issuing a more valuable token at a later stage, unless exposing yourself to a major risk of reward squatting.

i.e. You could not in future say we want to run a rewards period of of issuing an asset like WETH rewards for 10 days, after first initially issuing DAI as a reward. This hamstrings flexibility of the contract.

P.s. This is one of the slickest contracts I've read. Love how awesome it is.Just believe this should be fixed, then its good to go.

Tools Used

Manual analysis

Recommended Mitigation Steps

It is true you could probably write a script to manually go call 'claim' on thousands of squatting token addresses but this is a poor solution.

A simple mapping pattern could be used with an index mapping to a reward cycle with a reward token and a new accumulative etc. Users would likely need to be given a period a to claim from old reward cycles before their token balance could no longer reliably used to calculate past rewards. The would still be able to claim everything up until their last action (even though this may be before the rewards cycle ended).