Lack of ccipReceive validation

Summary

Since ccipReceive is the receiver of the message, it must be well validated to exclude malicious messages.

Vulnerability Details

The _ccipReceive function in WrappedTokenBridge.sol only checks the router when looking at the caller.
According to CCIP Best Practices (https://docs.chain.link/ccip/best-practices), it is necessary to check the source chain and sender for security reasons. (Sender depends on case).

function _ccipReceive(Client.Any2EVMMessage memory _message) internal override {

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/ccip/WrappedTokenBridge.sol#L234

Impact

May execute messages that the protocol does not expect.

Tools Used

Manual

Recommendations

Reinforce verification

It is possible that boostAmount may not be received depending on the set value of the protocol.

Summary

The maxBoost and maxLockingDuration used to calculate boostAmount can be set to any value at any time by the owner.
Depending on the user's input values, boostAmount may be set to zero, in which case the user loses his/her reward.

Vulnerability Details

maxBoost and maxLockingDuration have no upper or lower limits. Therefore, depending on the value of _amount and _lockingDuration, there are cases where the calculation result is zero.
This is especially a problem when a strange setting value is set even temporarily due to a misunderstanding or mistake by the management.

function getBoostAmount(uint256 _amount, uint64 _lockingDuration) external view returns (uint256) {
    if (_lockingDuration > maxLockingDuration) revert MaxLockingDurationExceeded();
    return (_amount * uint256(maxBoost) * uint256(_lockingDuration)) / uint256(maxLockingDuration);
}

https://github.com/Cyfrin/2023-12-stake-link/blob/549b2b8c4a5b841686fceb9c311dca9ac58225df/contracts/core/sdlPool/LinearBoostController.sol#L36-L39

Impact

User does not receive boostAmount even though he/she specified a lock period. It is inconsistent with the documentation and also the user loses the reward.

Tools Used

Manual

Recommendations

Validate realistic upper and lower limits when setting maxBoost and maxLockingDuration.

In the secondary chain, the UpdateLock process could be processed as InitiateUnlock.

Summary

In the secondary chain, the user's action enters queue and is finalized at execute time.
However, there is a possibility that the action in queue and the action in execute may differ.
As a result, the user may perform an unintended operation.

Vulnerability Details

When executing, the process is classified according to the difference between the previous amount and the boostAmount.
Looking at the conditional branch, if baseAmountDiff >= 0 and boostAmountDiff < 0, it is considered InitiateUnlock.

            int256 baseAmountDiff = int256(updateLockState.amount) - int256(curLockState.amount);
            int256 boostAmountDiff = int256(updateLockState.boostAmount) - int256(curLockState.boostAmount);
            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));
            } 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#L465-L483

However, looking at the boostAmount formula, if maxBoost becomes smaller or maxLockingDuration becomes larger, boostAmount may become negative even if _amount and _lockingDuration increase.

    return (_amount * uint256(maxBoost) * uint256(_lockingDuration)) / uint256(maxLockingDuration);

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

In other words, an update process that adds _amount or _lockingDuration may be treated as InitiateUnlock.

However, as long as the protocol and user use common-sense configuration values, they will not fall prey to this phenomenon.
Therefore, the actual risk from this phenomenon by itself is small.

Impact

An action is performed that is different from the user's intention. However, it is unlikely to happen naturally.

Tools Used

Manual

Recommendations

Instead of classifying actions by numerical increase or decrease, only actions that correspond to the actions put in the queue can be executed.

SDLPool has no storage gap

Summary

SDLPool is inherited by upgradeable contracts. (SDLPoolPrimary and SDLPoolSecondary).
Therefore, the contract implementation may change in the future.
However, since the storage gap is not implemented, it is not possible to increase the number of variables.
This limits the scope of future implementations.

Vulnerability Details

The SDLpool is inherited by the following contracts

        __SDLPoolBase_init(_name, _symbol, _sdlToken, _boostController);

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

    __SDLPoolBase_init(_name, _symbol, _sdlToken, _boostController);

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

Below is an example of OwnableUpgradeable.sol. This storage gap will ensure future scalability.

uint256[49] private __gap;

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/a40cb0bda838c2ef3dfc252c179f5c37c32e80c4/contracts/access/OwnableUpgradeable.sol#L94

Impact

Narrow future expansion possibilities

Tools Used

Manual

Recommendations

Add a storage gap at the end of the variable.