gkrastenov - Everyone can add an additional amount to an existing staking position

[sherlock-admin3]

gkrastenov

High

Everyone can add an additional amount to an existing staking position

Summary

Everyone can add an additional amount to an existing staking position.

Vulnerability Detail

To add an additional amount to an existing staking position through the addToPosition function, the caller must be the owner of the position. This check is done through the _requireOnlyOperatorOrOwnerOf modifier.

The _requireOnlyOperatorOrOwnerOf modifier always passes because the msg.sender is set as the owner and spender of the tokenId before calling the _isAuthorized function. The required checks in the _isAuthorized function are that spender != address(0) and owner == spender, which are always true.

    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        //@audit-issue H this is always true because:
        /* return
            spender != address(0) &&
            (owner == spender || isApprovedForAll(owner, spender) || _getApproved(tokenId) == spender);

        */
       //@audit-info here owner == spender == msg.sender

        // isApprovedOrOwner: caller has no rights on token
        require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");
    }

The _requireOnlyOperatorOrOwnerOf modifier does not prevent the addToPosition function from being called by another user. This can be problematic because, by design, only the owner or operator of the tokenId should be able to add an additional amount to the staking position and update the lockDuration and startLockTime.

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

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

Impact

The lockDuration and startLockTime will be updated, which may not be desired by the owner and can affect the staking.

Code Snippet

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

Tool used

Manual Review

Recommendation

Make the following changes:

+    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId, address nftOwner) internal view {
-    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        // isApprovedOrOwner: caller has no rights on token
-       require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");
+       require(ERC721Upgradeable._isAuthorized(nftOwner, msg.sender, tokenId), "FORBIDDEN");
    }

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

+       address nftOwner = ERC721Upgradeable.ownerOf(tokenId);
+       _requireOnlyOperatorOrOwnerOf(tokenId, nftOwner);
-       _requireOnlyOperatorOrOwnerOf(tokenId);

        require(amountToAdd > 0, "0 amount"); // addToPosition: amount cannot be null

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

Duplicate of #378