stake.link

Summary

A user can't initiate an unlock until 50% of their lock's duration has elapsed and then they can't actually withdraw for a period equal to half the duration after they initiate an unlock. Their tokens are only boosted for the period before they call initiateUnlock() in the applicable SDLPool contract.

But a malicious user can use the extendLockDuration() function to circumvent the intention of ensuring that a user has a period equal to at least 50% of their lock's duration between initiating unlock and actually unlocking, thereby lengthening the amount of time they stay boosted relative to people who did not use extendLockDuration().

This is because extendLockDuration() (via _storeUpdatedLock() and _updateLock()) resets your lock's startTime to the current block.timestamp and resets the lock's duration to the new duration you specified (and does not account for the time you already locked and were boosted). It basically treats it like a new lock with the new duration the user specified beginning at the block.timestamp when a user extended it. This allows you to remain boosted until you extend and then for half of your new extended lock's duration on top of that. See example below

This makes it possible for two people to lock for the exact same length of time overall but for one of them, who uses extendLockDuration(), to be boosted for a larger percentage of their lock duration while the person who does not use extendLockDuration() will be boosted for a lower percent of their lock's duration. See example

Example

Not using extendLockDuration:

1. Bob locks his tokens for 1 year, 6 months, and 1 minute. He is boosted.

2. Exactly halfway into his lock duration (9 months and 1/2 min)*, he initiates unlock. His boostAmount is decreased to 0.

3. Another 9 months and 1/2 minute goes by and he is able to unlock. He is boosted for only 50% of the time he is locked.

Using extendLockDuration()

1. Alice locks her tokens for 1 year at exactly the same block as Bob locked his tokens.

2. 6 months later, she extends her lock with a duration of 1 year and 1 minute (note: the extended lock duration must be longer than the initial lock to not revert updateLock() but it can be as little as 1 minute longer). Her total lock will be exactly the same length as Bob's initiated at exactly the same time.

3. Her lock.startTime is now the block.timestamp when she requested the extension and her lock.duration is 1 year and 1 minute

4. 6 months and 1/2 minute later (half her new lock.duration), she calls initiateUnlock(). This is 1 year and 1 minute into her total lock duration so she is boosted for 1 year and 1 minute out of the total lock period, whereas Bob was boosted for only 9 months and 1 minute of his lock duration, even though they both had the same overall lock duration.

5. Because of this Alice captures more rewards than Bob (even if they staked the exact same amount and though they had the same lock duration) because she is boosted for more of the time.

You can achieve the same thing by transferring tokens back and forth between accounts you control (which calls onTokenTransfer() which triggers the same flows through the same functions described above).

*Note: Bob could choose to wait longer to unlock but no matter when he unlocks, he will have to wait 9 months and 1/2 minute to unlock, whereas Alice will only have to wait 6 months and 1/2 minute to unlock because of her use of extendLockDuration()

Vulnerability Details

The extendLockDuration() function takes your lockId and a lock duration and calls _storeUpdatedLock():

The _storeUpdatedLock() function in turn calls updateLock() providing the owner, lockId, value (which is 0 for an extension), and lock duration you provided:

The updateLock() function resets your lock.startTime to the current block.timestamp if the _lockingDuration provided is not 0 (which it won't be for an extension). Note that the first if statement is not triggered because the example I gave you set the new lock duration to be 1 minute longer than the existing duration:

initiateUnlock() can only be called when 50% of the lock's duration has passed since its start time (note that in the case of a lock extension, the lock's duration will be the extension's duration and the start time will be when the extension was requested):

After a lock has been initiated, the user will be allowed to call withdraw() after a period equal to 50% of the lock's duration, neginning from lock.startTime.

Impact

Malicious users can call extendLockDuration() to stay boosted for a much higher percent of their lock compared to people who don't use extendLockDuration(). This allows them to get more rewards even for a stake of the exact same amount with a lock of the exact same length begun at exactly the same time.

Tools Used

Manual review

Recommendations

Revise the end of the 'updateLock()' function so that lock.startTime is not reset to the block.timestamp of the time of the extension or transfer - lock.startTime should be when the lock was originally initiated. Then calculate the amount of time of the lock that has elapsed so far and add the new locking duration to that and assign that result to lock.duration. This way, when someone who has extended or transferred their locks wants to initiate a withdrawal, half of their lock.duration will be half of their entire lock, not just half of their new lock.duration. Here is how I would revise the end of updateLock():

if (_lockingDuration != 0) {
if (lock.startTime = 0) {
lock.startTime = uint64(block.timestamp);
lock.duration = _lockingDuration;
}
else {
uint64 elapsedTime = uint64(block.timestamp) - lock.startTime;
lock.duration = _lockingDuration + elapsedTime;
}
else {
delete lock.startTime;
}

    lock.amount = baseAmount;
    lock.boostAmount = boostAmount;
    lock.expiry = 0;

    return lock;