qmdddd - When isUnlocked, the addToPosition function will set the position’s lockMultiplier to 0, but at the same time the lockDuration is greater than 0.

[sherlock-admin2]

qmdddd

Medium

When isUnlocked, the addToPosition function will set the position’s lockMultiplier to 0, but at the same time the lockDuration is greater than 0.

Summary

When isUnlocked, the addToPosition function will set the position’s lockMultiplier to 0, but at the same time the lockDuration is greater than 0, which is unreasonable and noncorresponding..

Vulnerability Detail

In the function addToPosition, user could add more tokens to position. The contract will recalculate the lock duration and update the lockMultiplier.

uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration)
    / (position.amount + amountToAdd);
    
position.startLockTime = _currentBlockTimestamp();
position.lockDuration = avgDuration;

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

However, this logic does not take into account the boundary case of isUnlocked. When isUnlocked, the function getMultiplierByLockDuration will return 0. But the position.lockDuration is still large than 0. This means that the user locks position for a period of time, but not receives a boost multiplier.

function getMultiplierByLockDuration(uint256 lockDuration) public view returns (uint256) {
    // in case of emergency unlock
    if (isUnlocked()) return 0;

    if (_maxLockDuration == 0 || lockDuration == 0) return 0;

    // capped to maxLockDuration
    if (lockDuration >= _maxLockDuration) return _maxLockMultiplier;

    return (_maxLockMultiplier * lockDuration) / (_maxLockDuration);
}

Impact

User locks his/her position for a period of time, but not receives a boost multiplier.

Code Snippet

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

Tool used

Manual Review

Recommendation

When isUnlocked, the position.lockDuration and position.lockMultiplier should be 0 together.

Duplicate of #138

[0xSmartContract]

This problem seems closer to Scenario 1. Because:

When the lock time expires (in the isUnlocked case), it is possible to have the multipliers drop to 0, but the lockDuration is still positive.

Similarly, in Scenario 1, when the lock time expires (remainingLockTime = 0), the multipliers remained the same.

Although the lock time in both cases expires, there are inconsistencies between the multipliers and the durations.

As a result, this problem is similar to Scenario 1. The lock time indicates a similar problem in terms of eliminating the imbalance between the multiplier and the duration,