zarkk01 - New ```LockingPosition``` can be created even if the pool has been unlocked in ```MlumStaking``` contract.

[sherlock-admin4]

zarkk01

High

New LockingPosition can be created even if the pool has been unlocked in MlumStaking contract.

Vulnerability Detail

Comments in MlumStaking contract state that no new lock can be set if the pool has been unlocked. However, the contract does not enforce this rule and allows users to create new LockingPosition even if the pool has been unlocked checking only if the lockDuration is 0. If lockDuration is 0, the amount will just be equal to amountWithMultiplier. We can see this here :

    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");
        }

        // ...
    }

This allows a user to create a LockingPosition even if the pool has been unlocked and actually increase the actual duration of the lock by just calling the addPosition later to the same position. We can see addPosition function here :

    /**
     * @dev Add to an existing staking position
     *
     * Can only be called by lsNFT's owner or operators
     */
    function addToPosition(uint256 tokenId, uint256 amountToAdd) external override nonReentrant {
        _requireOnlyOperatorOrOwnerOf(tokenId);
        require(amountToAdd > 0, "0 amount"); // addToPosition: amount cannot be null

        _updatePool();
        address nftOwner = ERC721Upgradeable.ownerOf(tokenId);
        _harvestPosition(tokenId, nftOwner);

        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);
        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);

        // handle tokens with transfer tax
        amountToAdd = _transferSupportingFeeOnTransfer(stakedToken, msg.sender, amountToAdd);

        // update position
        position.amount = position.amount + amountToAdd;
        _stakedSupply = _stakedSupply + amountToAdd;
        _updateBoostMultiplierInfoAndRewardDebt(position);

        emit AddToPosition(tokenId, msg.sender, amountToAdd);
    }

At the end, anyone can just create a new LockingPosition with a lockDuration non-zero even if the pool is unlocked.

Impact

This vulnerability leads to the bypass of the intention and invariant of the team that new LockingPositions must not be created when the pool is unlocked. Also, this could lead to unexpected staking behavior, introducing unfair advantages to certain users over the users who had locked during the locked pool.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider disallowing the creation of new LockingPosition when the NFT is unlocked by making this change :

    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");
+            revert();
        }
        // ...
    }

[0xSmartContract]

This issue is potentially considered a "low" security issue because it doesn't directly lead to a loss of funds or a serious security vulnerability.