stake.link

Summary

For boostAmount, the calculation result changes when the values of maxBoost and maxLockingDuration change. By monitoring the changes and adjusting boostAmount by updating the lock, it is possible to withdraw from the protocol (delete the lock) with effectiveBalances[user] remaining.
This allows users to receive rewards permanently without depositing any assets.

Vulnerability Details

Let's look at a specific example

1. first create a lock.

First, we try to create a lock with the following parameters.

At this time, the parameter of getBoostAmount is assumed to be the following.

This will create the following lock.
(Assuming that the lock is executed after queue)

effectiveBalances will be changed as follows.

effectiveBalances[_owner] = 300 (100 + boost:200)

2. Monitor maxBoost or maxLockingDuration updates by protocol. If advantageous, perform the update.

Suppose the protocol changes the maxLockingDuration to 5 year.

Then, update the lock as follows.

Since this is a secondary chain, the process goes into queue and the storage lock is not updated yet.

At this time, boostAmount is decreasing while duration is increasing. This phenomenon is very important.

3. execute update

In _executeQueuedLockUpdates, the process is classified by the difference from the status of one previous lock.

            int256 baseAmountDiff = int256(updateLockState.amount) - int256(curLockState.amount);
            int256 boostAmountDiff = int256(updateLockState.boostAmount) - int256(curLockState.boostAmount);

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L465-L466

The actual values are as follows

Then, since baseAmountDiff == 0 and boostAmountDiff < 0, it is classified as the following process.

            } else if (boostAmountDiff < 0) {
                locks[lockId].expiry = updateLockState.expiry;
                locks[lockId].boostAmount = 0;
                emit InitiateUnlock(_owner, lockId, updateLockState.expiry);

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L480-L483

The update process should have been performed, but InitiateUnlock was performed.
Therefore, locks[lockId].boostAmount = 0, but effectiveBalances[_owner] is not changed.(In truth, it should be -200).
Also, updateLockState.expiry = 0, so locks[lockId].expiry = 0.

After this process, locks[lockId] becomes the following.
（From the initial state, only the expiry and boostAmount are changed.)

effectiveBalances did not change, so they remain initial.
effectiveBalances[_owner] = 300 (100 + boost:200)

4. initiateUnlock

locks[lockId].expiry = 0, so we need to initiateUnlock.(Curiously, this would be initiateUnlock after InitiateUnlock).
There is a process to reduce effectiveBalances, but since lock.boostAmount is already 0, there is no change.

    uint64 expiry = uint64(block.timestamp) + halfDuration;
    lock.expiry = expiry;
    uint256 boostAmount = lock.boostAmount;
    lock.boostAmount = 0;
    effectiveBalances[msg.sender] -= boostAmount;
    totalEffectiveBalance -= boostAmount;

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L187-L193

The state of the lock at this time is as follows (still in the queue stage)

When this is executed, the following process is performed because baseAmountDiff==0 and boostAmountDiff==0.
Interestingly, despite the processing of initiateUnlock, the UpdateLock process is now being executed.

            } else {
                locks[lockId] = updateLockState;
                uint256 totalDiff = uint256(baseAmountDiff + boostAmountDiff);
                effectiveBalances[_owner] += totalDiff;
                totalEffectiveBalance += totalDiff;
                emit UpdateLock(

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L484-L489

Since totalDiff is zero, effectiveBalances[_owner] does not change.
In other words, it remains as below.
effectiveBalances[_owner] = 300 (100 + boost:200)

5. withdraw

All that's left is to wait for the lock period to pass and withdraw it.

At this time, effectiveBalances[msg.sender] finally decreases.
However, it is only by the amount of lock.amount(=100).

    uint256 baseAmount = lock.amount;
    if (_amount > baseAmount) revert InsufficientBalance();
    lock.amount = baseAmount - _amount;
    effectiveBalances[msg.sender] -= _amount;
    totalEffectiveBalance -= _amount;

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L227-L232

In other words, effectiveBalances are as follows.
effectiveBalances[_owner] = 200 (0 + boost:200).

When withdraw is executed, the lock is removed and the sdlToken is returned.

            if (baseAmountDiff < 0) {
                emit Withdraw(_owner, lockId, uint256(-1 * baseAmountDiff));
                if (updateLockState.amount == 0) {
                    delete locks[lockId];
                    delete lockOwners[lockId];
                    balances[_owner] -= 1;
                    if (tokenApprovals[lockId] != address(0)) delete tokenApprovals[lockId];
                    emit Transfer(_owner, address(0), lockId);
                } else {
                    locks[lockId].amount = updateLockState.amount;
                }
                sdlToken.safeTransfer(_owner, uint256(-1 * baseAmountDiff));

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/SDLPoolSecondary.sol#L468-L479

As a result, effectiveBalances[_owner] = 200 will remain forever.
In this example, the amount is small for the sake of clarity, but it is very dangerous because if the amount is large, the owner can receive a very large reward as much as he/she wants.

Impact

Receive large rewards forever without risk.

Tools Used

Manual

Recommendations

This complex attack path is a combination of several characteristics.

• The protocol's configurable values that reduce boostAmount.
  There are solutions such as fixing maxLockingDuration and maxBoost.

• It is possible to extend duration by 1 sec.
  There is no use for this except for abuse. There is a way for the protocol to provide several fixed durations and let the user choose from them.

• The action that the user puts in the queue and the action that is executed are different.
  Instead of classifying execute actions by increasing/decreasing amount, there is a way to manage them by initiateUnlock flag, update flag, etc.

• Timing of increase/decrease of effectiveBalances depends on the operation.
  effectiveBalances increase/decrease timing is divided into two types: initiateUnlock when queued and update when executed. Either of them should be unified.

• A user can delete the lock even though the effectiveBalances have not been sufficiently reduced.
  effectiveBalances added by lock should be reduced by that amount before lock can be deleted.
  In addition to managing by effectiveBalances[user], it is also possible to manage by lockEffectiveBalance[lockId], etc.

The solutions are manifold, but in any case, we must be checked after modification to ensure that this deadly attack path does not remain.