coffiasd - malicious can by pass the operator or owner check

[sherlock-admin4]

coffiasd

High

malicious can by pass the operator or owner check

Summary

_requireOnlyOperatorOrOwnerOf function use msg.sender instead of tokenId owner lead to malicious by pass the OnlyOperatorOrOwnerOf check.

Vulnerability Detail

from code link

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

OZ code ERC721Upgradeable.sol

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

from above code we can the first parameter of _isAuthorized is the owner of tokenID, however the protocol use msg.sender which lead to owner == spender check always return true.

test:

    function testByPassOperaterOrOwnerCheck() public {
        _stakingToken.mint(BOB, 2 ether);
        _stakingToken.mint(ALICE, 2 ether);

        //alice create position.
        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.createPosition(1 ether, 1 days);
        vm.stopPrank();

        //eq tokenID owner
        assertEq(_pool.ownerOf(1),ALICE);

        vm.startPrank(BOB);
        _stakingToken.approve(address(_pool), 1 ether);
        _pool.addToPosition(1, 1 ether);
        vm.stopPrank();
    }

Impact

malicious can by pass the operator or owner check

Code Snippet

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

Tool used

Foundry
Manual Review

Recommendation

     /**
      * @dev Check if a userAddress has privileged rights on a spNFT
      */
     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(_ownerOf(tokenId), msg.sender, tokenId), "FORBIDDEN");//@audit @<
     }

Duplicate of #378