July: Bug adding funds to ongoing rewards

[bingen]

Thanks @willjgriff for pointing this out!

If somebody adds more rewards to the contract (it is unprotected), the distribution would be disrupted (no funds would be lost though).

https://github.com/aragonone/Unipool/blob/master/contracts/Unipool.sol#L139

Using his example:

Original amount deposited = 30 for 30 days.
At day 20, 1 more token is deposited.

remaining = 10 days (out of 30). leftover = 10 * 1 (current rate is 1 per day). rewardRate = 1 + 10 / 30 = 0.366

rewardRate was previously 1, but for the final 10 days the reward rate will only be 0.366. The period would be extended for another 20 days, so liquidity providers could still get all the rewards, but waiting longer (so there may be indirect losses in terms of opportunity costs, or something like that)

At a first glance 2 ideas come to my mind:

• Whitelisting the _from param
• Making sure that the new reward rate is >= that the previous one, so:
  • Current liquidity providers can only earn more, even if they stick to the initial period
  • The amount needed to “disrupt” is not negligible (right now it could be just 1 wei of a token), so it wouldn’t make sense to try to do it maliciously
    The second one seems better for the following reasons:
  • We keep the contract more flexible and generic, as anybody can pour rewards in
  • The first one doesn't fully fix the issue from the user perspective: you have to trust that the owner won't exploit it to force you to stay providing liquidity longer, changing the rules on the fly. Of course we wouldn't do that, but it's still a bad design.