zarkk01 - _requireOnlyOperatorOrOwnerOf does not correctly check the owner or the operator of the position leading to anyone can adjust the duration of a LockingPosition by adding to it.

[sherlock-admin4]

zarkk01

High

_requireOnlyOperatorOrOwnerOf does not correctly check the owner or the operator of the position leading to anyone can adjust the duration of a LockingPosition by adding to it.

Vulnerability Detail

The requireOnlyOperatorOrOwnerOf function is supposed to check if the caller of addPosition in MlumStaking contract is, actually, the owner of the LockingPosition or authorized. However, in the way that the function call of _isAuthorized call is implemented the requireOnlyOperatorOrOwnerOf will always return true. We can see the the _isAuthorized function of ERC721 here :

    /**
     * @dev Returns whether `spender` is allowed to manage `owner`'s tokens, or `tokenId` in
     * particular (ignoring whether it is owned by `owner`).
     *
     * 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);
    }

In MlumStaking, msg.sender is passed in both owner and spender params without checking if the msg.sender is the owner of the NFT as stated in the comments of the _isAuthorized function of ERC721. This results to anyone can call addPosition function for whichever NFT they want to. By adding to the position, an attacker can adjust the duration of any NFT position and can prevent the actual owner of the NFT to withdraw their funds or vote in Voter contract.

Impact

Anyone can change the duration of a LockingPosition can lead to a DoS attack on the actual owner of the position since the lockDuration of the position was selected by the owner so to serve his needs. By extending or reducing the duration of the position, the attacker can prevent the owner from withdrawing his funds or voting in the Voter contract among other problems for the actual owner which does not equal the extra amount in the position that the attacker added.

Proof of concept

This PoC demonstrates the scenario where an attacker DoS the withdrawal of the actual owner of the LockingPosition by adding to it a very tiny amount and extending the duration of it.
To understand better this vulnerability, add this test in MlumStakingTest.sol and run forge test --mt testWithdrawDOSbyAddingToPosition:

function testWithdrawDOSbyAddingToPosition() public {
        _stakingToken.mint(ALICE, 100 ether);
        _stakingToken.mint(BOB, 1 ether);

        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), 50 ether);
        _pool.createPosition(1 ether, 2 days);
        vm.stopPrank();

        skip(1 days);

        vm.expectRevert();
        vm.prank(ALICE);
        _pool.withdrawFromPosition(1, 0.5 ether);

        skip(1 days);

        // as long as the malicious Bob deposits amountAdd > amountStaked / (secondsInitDuration - 1) the withdraw of Alice will fail because the duration gonna be extended.
        vm.startPrank(BOB);
        _stakingToken.approve(address(_pool), 5787070527029);
        _pool.addToPosition(1, 5787070527029);
        vm.stopPrank();

        vm.expectRevert();
        vm.prank(ALICE);
        _pool.withdrawFromPosition(1, 1 ether);
    }

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider making this change in the _requireOnlyOperatorOrOwnerOf function so to implement correctly the check :

    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(ERC721Upgradeable.ownerOf(tokenId), msg.sender, tokenId), "FORBIDDEN");
    }

Duplicate of #378