onthehunt - unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; will be incorrectly set in notifyDelegation() in the specified order

[sherlock-admin4]

onthehunt

Medium

unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; will be incorrectly set in notifyDelegation() in the specified order

Summary

We set the unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; in Distribute#notifyDelegation(),

specifically when bool newDelegation is true as in this piece of code,

if (newDelegation) {
            unclaimed[delegator].delegatees.add(delegatee);
            unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch;
}

This is due to this equating to true -> delegations[delegatee][_msgSender()] == amount.
Which will be the case when you delegate tokens for the first time and that's not an issue,
but if you do the following actions step by step ->
L2Staking#delegateStake(), L2Staking#undelegateStake(), L2Staking#claimUndelegation(),
(you don't claim rewards, just claiming back the initial tokens we've deposited to the delegatee),
and call L2Staking#delegateStake() for a second time, you will rewrite the previous
unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch;, if we delegate to the same delegatee both of the times.

It will be treated as a new delegation and if you want to then claim the rewards via L2Staking#claimReward() for the specified delegatee , it will start from the epoch that was ongoing in the second call of the L2Staking#delegateStake()
due to bool newDelegation being true and overwriting unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch;.

The second time we call L2Staking#delegateStake() , bool newDelegation is true is because we track it via delegations[delegatee][_msgSender()] == amount and in L2Staking#undelegateStake() we already deleted it through this line of code -> delete delegations[delegatee][_msgSender()];

Because we deleted the previous delegation in the LOC above (delete delegations[delegatee][_msgSender()];) in L2Staking#undelegateStake() , and now during the second invocation of L2Staking#delegateStake(),
this will equate to true -> delegations[delegatee][_msgSender()] == amount and consequently,
bool newDelegation will be again as a truthy value, which will cause unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; to be rewritten in the second time we invoke L2Staking#delegateStake(),

Root Cause

We do not send the reward tokens programmatically back to the user in the code, they are to be claimed manually, and rewriting of the unclaimed[delegator].unclaimedStart[delegatee] will render the rewards from the previous epoches unclaimable, because in the Distribute#_claim(), the for loop takes the unclaimed[delegator].unclaimedStart[delegatee] as a starting value to loop over

Just a small note if you somehow think about it while reading the code:
The number of undelegateLockEpochs that's used in L2Staking#undelegateStake() doesn't matter to the vulnerability.

If its set to 5 epochs, we will have to wait for the 5 epochs to pass to be able to call claimUndelegation(),
and it will still rewrite the unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; as explained,
when we call L2Staking#delegateStake() for a second time.

Internal pre-conditions

All of the used functions in the example need to be using the same delegatee (both L2Staking#delegateStake() invocations, L2Staking#undelegateStake(), L2Staking#claimReward())

External pre-conditions

No response

Attack Path

1. Alice decides to stake her 100 morph tokens to the delegatee named Bob using L2Staking#delegateStake(Bob, 100).
   
2. 5 epochs pass, Alice decides that she needs those 100 morph tokens, so Alice calls L2Staking#undelegateStake(Bob).
   
3. Alice waits the specified undelegateLockEpochs and calls L2Staking#claimUndelegation() to claim her initial staked tokens.
   3.1 Note that L2Staking#claimUndelegation() gives your initially staked tokens back only, the reward from the staked assets is
   claimed via L2Staking#claimReward(Bob).
   
4. Alice can claim her rewards, but instead of claiming them now,
   she decides to call L2Staking#delegateStake(Bob, 100) again to stake her 100 morph tokens.
   4.1 This is where the vulnerability arises, Alice haven't claimed her rewards and she calls L2Staking#delegateStake(Bob, 100) a second time, which will overwrite the unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; to a new value, thus making the claiming of rewards accrued in the epochs from between the first L2Staking#delegateStake() and L2Staking#undelegateStake() that are described
   in Step 1. and Step 2. impossible as the for loop in Distribute#_claim()
   (Distribute#_claim() is called from Distribute#claim(), which is called
   from L2Staking.claimReward(delegatee, lastEpochIndex).
5. 3 epochs pass and Alice decides that now its time to claim her rewards via L2Staking#claimReward(Bob, targetEpochIndex), but she can only claim the rewards only from the last 3 epochs due to unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; being rewritten to a new epoch in as explained in Step 4.

Impact

Alice can't claim her rewards from the staked assets if she calls L2Staking#delegateStake(Bob, 100) while having pending rewards, which will be case in our example, due to unclaimed[delegator].unclaimedStart[delegatee] = effectiveEpoch; being rewritten to a new epoch.

PoC

No response

Mitigation

When a user invokes L2Staking#undelegateStake(), execute the L2Staking#claimReward() function in it, although we need to make L2Staking#claimReward public instead of its current access modifier external.