nikhil840096 - Access Control Vulnerability in MlumStaking.sol:addToPosition

[sherlock-admin3]

nikhil840096

Medium

Access Control Vulnerability in MlumStaking.sol:addToPosition

Summary

The addToPosition function in the MlumStaking.sol contract allows any user to add tokens to an existing staking position by providing the tokenId of the position. This introduces a significant security risk, as malicious actors can manipulate other users positions.

Vulnerability Detail

addToPosition function directly depends on function _requireOnlyOperatorOrOwnerOf for checks, while the function _requireOnlyOperatorOrOwnerOf is implemented incorrectly and is vulnerable.
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L140-L143
requireOnlyOperatorOrOwnerOf function calls ERC721Upgradeable._isAuthorized with parameter for owner and sender as msg.sender and mseg.sender.

While if we look at the implementation of _isAuthorized function it is this

 // WARNING: This function assumes that `owner` is the actual owner of `tokenId` and does not verify this assumption.

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

The warning is given above that it actually assumes that owner given is the owner of the tokenId. So here the function fails to protect addToPosition from access control vulnerability.

Impact

There are some impacts as given below :

1. adding tokens to another user's position, an attacker can manipulate the average stake time, causing the user's position to take longer to unlock.
2. The unauthorized addition of tokens increases the total amount staked, potentially affecting the user's strategy and plans. The control over one's staking position is compromised, leading to a loss of confidence in the protocol's security.
3. Function natspec states that it can be also callable by operator , but there is no check for the operator.

Code Snippet

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

Tool used

Manual Review

Recommendation

• There is check required in requireOnlyOperatorOrOwnerOf function for tokenId owner, and also for operator

Duplicate of #378