dany.armstrong90 - The lockMultiplier can be decreased unexpectedly by renewLockPosition.

[sherlock-admin4]

dany.armstrong90

Medium

The lockMultiplier can be decreased unexpectedly by renewLockPosition.

Summary

After MlumStaking.sol#createPosition(), the lockMultiplier is not decreased when addToPosition(), renewLockPosition() and extendLockPosition() are called individually unless multiplier settings are not changed.
And in addToPosition(), the lockMultiplier is not decreased but lockDuration can be decreased.
And in renewLockPosition(), lockMultiplier is calculated from lockDuration.
So if a user calls through addToPosition() => renewLockPosition(), lockMultiplier can be decreased unexpectedly so amountWithMultiplier is decreased and less rewards are distributed to the user.

Vulnerability Detail

MlumStaking.sol#createPosition() function is as follows.

    function createPosition(uint256 amount, uint256 lockDuration) external override nonReentrant {
        // no new lock can be set if the pool has been unlocked
        if (isUnlocked()) {
            require(lockDuration == 0, "locks disabled");
        }

        _updatePool();

        // handle tokens with transfer tax
        amount = _transferSupportingFeeOnTransfer(stakedToken, msg.sender, amount);
        require(amount != 0, "zero amount"); // createPosition: amount cannot be null

        // mint NFT position token
        uint256 currentTokenId = _mintNextTokenId(msg.sender);

        // calculate bonuses
370     uint256 lockMultiplier = getMultiplierByLockDuration(lockDuration);
371     uint256 amountWithMultiplier = amount * (lockMultiplier + 1e4) / 1e4;

        // create position
        _stakingPositions[currentTokenId] = StakingPosition({
            initialLockDuration: lockDuration,
            amount: amount,
            rewardDebt: amountWithMultiplier * (_accRewardsPerShare) / (PRECISION_FACTOR),
            lockDuration: lockDuration,
            startLockTime: _currentBlockTimestamp(),
380         lockMultiplier: lockMultiplier,
381         amountWithMultiplier: amountWithMultiplier,
            totalMultiplier: lockMultiplier
        });

        // update total lp supply
        _stakedSupply = _stakedSupply + amount;
        _stakedSupplyWithMultiplier = _stakedSupplyWithMultiplier + amountWithMultiplier;

        emit CreatePosition(currentTokenId, amount, lockDuration);
    }

Here, lockMultiplier is calculated from lockDuration.
And MlumStaking.sol#addToPosition() function is as follows.

    function addToPosition(uint256 tokenId, uint256 amountToAdd) external override nonReentrant {
        ...

        StakingPosition storage position = _stakingPositions[tokenId];

        // we calculate the avg lock time:
        // lock_duration = (remainin_lock_time * staked_amount + amount_to_add * inital_lock_duration) / (staked_amount + amount_to_add)
        uint256 remainingLockTime = _remainingLockTime(position);
 410    uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration)
            / (position.amount + amountToAdd);

        position.startLockTime = _currentBlockTimestamp();
414     position.lockDuration = avgDuration;

        // lock multiplier stays the same
417     position.lockMultiplier = getMultiplierByLockDuration(position.initialLockDuration);

        ...
    }

As we can see on L410, avgDuration < position.initialLockDuration so lockDuration is decreased.
But on L417, lockMultiplier is calculated from position.initialLockDuration so it is not decreased.
And MlumStaking.sol#renewLockPosition(), _lockPosition() functions are as follows.

    function renewLockPosition(uint256 tokenId) external nonReentrant {
        _requireOnlyApprovedOrOwnerOf(tokenId);

        _updatePool();
@>     _lockPosition(tokenId, _stakingPositions[tokenId].lockDuration, false);
    }
    function _lockPosition(uint256 tokenId, uint256 lockDuration, bool resetInitial) internal {
        require(!isUnlocked(), "locks disabled");

        StakingPosition storage position = _stakingPositions[tokenId];

        // for renew only, check if new lockDuration is at least = to the remaining active duration
        uint256 endTime = position.startLockTime + position.lockDuration;
        uint256 currentBlockTimestamp = _currentBlockTimestamp();
        if (endTime > currentBlockTimestamp) {
            require(lockDuration >= (endTime - currentBlockTimestamp) && lockDuration > 0, "invalid");
        }

        // for extend lock postion we reset the initial lock duration
        // we have to check that the lock duration is greater then the current
        if (resetInitial) {
            require(lockDuration > position.initialLockDuration, "invalid");
            position.initialLockDuration = lockDuration;
        }

        _harvestPosition(tokenId, msg.sender);

        // update position and total lp supply
        position.lockDuration = lockDuration;
714     position.lockMultiplier = getMultiplierByLockDuration(lockDuration);
        position.startLockTime = currentBlockTimestamp;
        _updateBoostMultiplierInfoAndRewardDebt(position);

        emit LockPosition(tokenId, lockDuration);
    }

As you can see on L714, the lockMultiplier is calculated from lockDuration.

If a user calls renewLockPosition() => _lockPosition() after addToPosition() by some reason, the lockMultiplier is decreased.
This results in distribution of less rewards to the user.

Impact

If a user calls renewLockPosition() after addToPosition(), the lockMultiplier is decreased so the amountWithMultiplier is decreased and the user receives less rewards than normal unexpectedly.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L714

Tool used

Manual Review

Recommendation

MlumStaking.sol#_lockPosition() function has to be modified as follows.

    function _lockPosition(uint256 tokenId, uint256 lockDuration, bool resetInitial) internal {
        require(!isUnlocked(), "locks disabled");

        StakingPosition storage position = _stakingPositions[tokenId];

        // for renew only, check if new lockDuration is at least = to the remaining active duration
        uint256 endTime = position.startLockTime + position.lockDuration;
        uint256 currentBlockTimestamp = _currentBlockTimestamp();
        if (endTime > currentBlockTimestamp) {
            require(lockDuration >= (endTime - currentBlockTimestamp) && lockDuration > 0, "invalid");
        }

        // for extend lock postion we reset the initial lock duration
        // we have to check that the lock duration is greater then the current
        if (resetInitial) {
            require(lockDuration > position.initialLockDuration, "invalid");
            position.initialLockDuration = lockDuration;
        }

        _harvestPosition(tokenId, msg.sender);

        // update position and total lp supply
        position.lockDuration = lockDuration;
-       position.lockMultiplier = getMultiplierByLockDuration(lockDuration);
+       position.lockMultiplier = getMultiplierByLockDuration(position.initialLockDuration);
        position.startLockTime = currentBlockTimestamp;
        _updateBoostMultiplierInfoAndRewardDebt(position);

        emit LockPosition(tokenId, lockDuration);
    }

Duplicate of #138