PUSH0 - Incorrect access control for _requireOnlyOperatorOrOwnerOf(). Anyone can call MlumStaking.addToPosition() for other users, with various impacts.

[sherlock-admin2]

PUSH0

Medium

Incorrect access control for _requireOnlyOperatorOrOwnerOf(). Anyone can call MlumStaking.addToPosition() for other users, with various impacts.

Summary

The function MlumStaking.addToPosition() allows a user to add more MLUM to their position. This also has an effect of increasing their lock time. The function seem to have access control as an internal function _requireOnlyOperatorOrOwnerOf(tokenId).

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

However, the access control is incorrectly implemented, allowing anyone to access the function for other users.

We show two different resulting attacks and their impacts.

Vulnerability Detail

The function _requireOnlyOperatorOrOwnerOf() is implemented as follow:

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/main/magicsea-staking/src/MlumStaking.sol#L140-L143

The ERC721Upgradeable._isAuthorized() function is implemented as follow:

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/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC721/ERC721Upgradeable.sol#L215-L219

Because _isAuthorized() is called with the same owner and spender being msg.sender, and said address is never zero, authorization will always return true due to spender != address(0) and owner == spender always evaluating to true.

Therefore the function MlumStaking.addToPosition() has no effective access control or ownership checks, allowing anyone to add their own MLUM to others' position. Normally this is not a problem, but because addToPosition() has an effect of modifying one's lock duration as well, the action now other implications and enabling other attacks, which we describe below.

PoC

We describe two different attacks arising from unauthorized locking with examples.

Denying a position's withdrawal by adding to their lock.

• Alice has a lock duration of 1 year and 50 MLUM, that is expiring.
• Bob denies Alice's withdrawal by adding another MLUM to Alice's position.
• Alice's position is forced an extension of about (1 year)/50 ~ 7.3 days.

Alice's lock position has been forced an extension against their will.

Denying a position's voting rights by adding to their lock and modify their lockDuration.

• Alice has a lock duration of at least 3 months and 50 MLUM. This gives her the power to vote.
• This position is close to expiring (about a few days left). However, because throughout those 3 months, Alice never touched the position, its attributes initialLockDuration and lockDuration stays at 3 months.
  • Alice still retains the right to vote.
• Bob denies this right by locking 1 wei into Alice's position. The position's lockDuration is now a few days, and attempts to vote will fail the following check:

if (_mlumStaking.getStakingPosition(tokenId).lockDuration < _periodDuration) {
    revert IVoter__InsufficientLockTime();
}

https://github.com/sherlock-audit/2024-06-magicsea/blob/main/magicsea-staking/src/Voter.sol#L175-L177

Alice's position has been denied voting power that it should retains if there were access control.

Coded PoC

We provide a coded PoC to prove the incorrect access control:

Coded PoC

Run the PoC with forge test --match-test testBobAddsToAlicePosition. The PoC shows that BOB can lock ALICE's position despite having no approvals.

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

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

    // check lockduration
    MlumStaking.StakingPosition memory position = _pool.getStakingPosition(1);
    assertEq(position.lockDuration, 1 days);

    skip(43200);

    // add to position should take calc. avg. lock duration
    vm.startPrank(BOB);
    _stakingToken.approve(address(_pool), 1 ether);
    _pool.addToPosition(1, 1 ether);
    vm.stopPrank();

    position = _pool.getStakingPosition(1);

    assertEq(position.lockDuration, 64800);
}

Impact

addToPosition()'s access control is ineffective, anyone can grief other people's positions. Impacts include (but potentially not limited to) denying withdrawals by lengthening the other's position, or denying votes by shortening their lockDuration attribute.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

The intended check is likely:

require(ERC721Upgradeable._isAuthorized(_ownerOf(tokenId), msg.sender, tokenId), "FORBIDDEN");

Or the correct access control should have been _requireOnlyApprovedOrOwnerOf() instead.

Duplicate of #378