WJLP loses unclaimed rewards when updating user's rewards

[code423n4]

Handle

kenzo

Vulnerability details

After updating user's rewards in _userUpdate, if the user has not claimed them, and _userUpdate is called again (eg. on another wrap), the user's unclaimed rewards will lose the previous unclaimed due to wrong calculation.

Impact

Loss of yield for user.

Proof of Concept

When updating the user's unclaimedJoeReward, the function doesn't save it's previous value. (Code ref)

        if (user.amount > 0) {
            user.unclaimedJOEReward = user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt);
        }
        if (_isDeposit) {
            user.amount = user.amount.add(_amount);
        } else {
            user.amount = user.amount.sub(_amount);
        }
        // update for JOE rewards that are already accounted for in user.unclaimedJOEReward
        user.rewardDebt = user.amount.mul(accJoePerShare).div(1e12);

So for example, rewards can be lost in the following scenario. We'll mark "acc1" for the value of "accJoePerShare" at step 1.

1. User Zebulun wraps 100 tokens. After _userUpdate is called: unclaimedJOEReward = 0, rewardDebt = 100*acc1.
2. Zebulun wraps 50 tokens: unclaimedJOEReward = 100acc2 - 100acc1, rewardDebt = 150 * acc2.
3. Zebulun wraps 1 token: unclaimedJOEReward = 150acc3 - 150acc2, rewardDebt = 151*acc3 So in the last step, Zebulun's rewards only take into account the change in accJoePerShare in steps 2-3, and lost the unclaimed rewards from steps 1-2.

Recommended Mitigation Steps

Change the unclaimed rewards calculation to:

user.unclaimedJOEReward = user.unclaimedJOEReward.add(user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt));

[kingyetifinance]

@LilYeti : Probably severity 3 due to loss of allocation of funds for other users wrapping LP tokens.

[0xtruco]

Actually not a problem, the accrued reward is updated as time goes on and should be overridden. Follows same logic from Master chef v2 here: https://snowtrace.io/address/0xd6a4F121CA35509aF06A0Be99093d08462f53052#code

[alcueca]

@0xtruco, are you sure of what you are saying? To me the issue seems to still exist. Could you elaborate?

[0xtruco]

@alcueca Yes you are correct this actually is an issue. I initially thought that we were harvesting rewards just like TJ when users wrapped tokens but turns out that line is not there. This is in the new version of our code but was not in the version submitted at the time of the contest. Thanks for looking into it more! Back to high risk.