ChinmayF - Anyone can add to someone's else staking position in MLUMStaking and potentially extend their lock duration

[sherlock-admin4]

ChinmayF

Medium

Anyone can add to someone's else staking position in MLUMStaking and potentially extend their lock duration

Summary

The addToPosition() function is used to add to an already exisiting staking position, which adds to staked amount and changes startLockTime as well as potentially change the lockDuration.

It is intended to restrict this function to be only callable by the NFT owners(or approved address), but this restriction does not work and anyone can come and add to a position, and extend the duration without the staker's permission.

Vulnerability Detail

In addToPosition(), first line has _requireOnlyOperatorOrOwnerOf(tokenId) which signals that it is intended to only allow NFT's owner or approved address to modify a position. see here.

Also, the comment above addToPosition() says,"Can only be called by lsNFT's owner or operators"

So it is clear that adding to a position is restricted. But this restriction fails completely due to incorrect logic in the _requireOnlyOperatorOrOwnerOf() function.

   function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");
    }

ERC721Upgradeable._isAuthorized() function :

    function _isAuthorized(address owner, address spender, uint256 tokenId) internal view virtual returns (bool) {
        return
            spender != address(0) &&
            (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender);
    }

We can clearly see that when the owner and sender parameters passed to the _isAuthorized() function are same it will return true. In our case, _isAuthorized will always return true because msg.sender is passed for both the parameters, and thus _requireOnlyOperatorOrOwnerOf will always go through.

This means there is no restriction placed by the function. Any msg.sender who wants to call addToPosition() can freely do it and it will always allow.

Now a call to addToPosition(tokenID) alters the startLockTime and can also extend the lock based on the dynamics of the avgDuration calculation. This is done without the permission of the original staker, and will keep him from unlocking his staking position longer than he wanted to, and prevent him from accessing his own staked assets even after the lock was expected to have expired and withdrawable.

Impact

Intended functionality of restricting random people from adding to others' position is broken, and can also potentially extend the locked time of the position.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L142

Tool used

Manual Review

Recommendation

Change the _requireOnlyOperatorOrOwnerOf() function to following :

    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        address _owner = _ownerOf(tokenID);
        require(ERC721Upgradeable._isAuthorized(_owner, msg.sender, tokenId), "FORBIDDEN");
    }

Duplicate of #378