Users' rewards in Wrapped JLP will be miscalculated & lost. Every interaction with WJLP (wrap, unwrapFor...) will trigger the bug.
Location
Function _userUpdate in WJLP.sol
Explanation of the bug
So the nature of this WJLP is simply a wrap of the JLP, and it itself maintains certain information regarding the rewards that each user is entitled to. This information must be updated whenever there is a change in balances of any users or any users who want to redeem their JOE rewards. The function that was used to do this is _userUpdate
This function has a very similar implementation to how TraderJoe (MasterchefV2) did it, but there is a subtle but significant difference: Whenever TraderJoe updates the pending and rewardDebt information, they always immediately transfer out all pending rewards. However, in YETI's contracts, the rewards is not transferred out immediately but instead accumulated and only transfer when the user calls claimReward.
As such, this leads to a miscalculation of users' rewards, and eventually, users will lose their rewards.
Line 252 of WJLP.sol, the unclaimedJOEReward should be accumulated (+=) instead of just reassigning like TraderJoe, OR the reward should be transferred out immediately.
Proof of Concept
For simplicity, I will ignore the 1e12 division.
Let's consider a new user who has 0 WJLP.
Let's denote the action of calling wrap(X, msg.sender, msg.sender, msg.sender) as deposit X
At T0
user.amount = 0, accJoePerShare=1, user.rewardDebt = 0
At T1 (a while after T0):
user deposits 100 JLP
Line 248: accJoePerShare = 2
Line 252: (skipped) => user.unclaimedJOEReward = 0
Line 252: user.unclaimedJOEReward = 200*3.01-600 = 2
Line 256: user.amount = 200 + 100 = 300
Line 262: user.rewardDebt = 300 * 3.01 = 903
=> If the user claims reward here, he will get 2, so he lost 98 units of rewards and it's not recoverable.
Recommended Mitigation Steps
To mitigate, either change the formula of line 252 to user.unclaimedJOEReward = unclaimedJOEReward.add(user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt));, or simply do it as JOE, transfer out all the accuredReward at every updates
kingyetifinance
commented
2 years ago
Handle
UncleGrandpa925
Vulnerability details
Impact
Users' rewards in Wrapped JLP will be miscalculated & lost. Every interaction with WJLP (wrap, unwrapFor...) will trigger the bug.
Location
Function
_userUpdateinWJLP.solExplanation of the bug
So the nature of this WJLP is simply a wrap of the JLP, and it itself maintains certain information regarding the rewards that each user is entitled to. This information must be updated whenever there is a change in balances of any users or any users who want to redeem their JOE rewards. The function that was used to do this is
_userUpdateThis function has a very similar implementation to how TraderJoe (MasterchefV2) did it, but there is a subtle but significant difference: Whenever TraderJoe updates the
pendingandrewardDebtinformation, they always immediately transfer out all pending rewards. However, in YETI's contracts, the rewards is not transferred out immediately but instead accumulated and only transfer when the user callsclaimReward.As such, this leads to a miscalculation of users' rewards, and eventually, users will lose their rewards.
Line 252 of
WJLP.sol, theunclaimedJOERewardshould be accumulated (+=) instead of just reassigning like TraderJoe, OR the reward should be transferred out immediately.Proof of Concept
For simplicity, I will ignore the 1e12 division.
Let's consider a new user who has 0 WJLP. Let's denote the action of calling
wrap(X, msg.sender, msg.sender, msg.sender)asdeposit XAt T0 user.amount = 0, accJoePerShare=1, user.rewardDebt = 0
At T1 (a while after T0): user deposits 100 JLP
At T2 (a while after T1): user deposits 100 JLP
=> If user claims reward here, he will get 100
At T3 (shortly after T2): user deposits 100 JLP
=> If the user claims reward here, he will get 2, so he lost 98 units of rewards and it's not recoverable.
Recommended Mitigation Steps
To mitigate, either change the formula of line 252 to
user.unclaimedJOEReward = unclaimedJOEReward.add(user.amount.mul(accJoePerShare).div(1e12).sub(user.rewardDebt));, or simply do it as JOE, transfer out all the accuredReward at every updates