0xboriskataa - An attacker can prevent anyone from unstaking in MlumStaking.sol

[sherlock-admin3]

0xboriskataa

High

An attacker can prevent anyone from unstaking in MlumStaking.sol

Summary

MlumStaking.sol: addToPosition() can be used by a user to increase his staked amount. It calls _requireOnlyOperatorOrOwnerOf() which should restrict the access to this function and allow only the owner of the staking position to call it. However anyone can still do so.

Vulnerability Detail

Let's look at the implementation of _requireOnlyOperatorOrOwnerOf():

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

It calls _isAuthorized on the ERC721Upgradeable contract:

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

The warning message in the comments explains that this function doesn't verify if owner is the actual owner of the token (staking position).

_requireOnlyOperatorOrOwnerOf() calls this function with msg.sender for both the owner and the spender parameter. With these inputs _isAuthorized() will return true:

spender != address(0) (true) and owner == spender (true).

As you can see no matter who msg.sender is this will always return true.
This means that there is no access control whatsoever and even if you are not the owner of the staking position you can still call addToPosition().

Impact

Here is part of the implementation for addToPostion():

function addToPosition(uint256 tokenId, uint256 amountToAdd) external override nonReentrant {
        _requireOnlyOperatorOrOwnerOf(tokenId);
        require(amountToAdd > 0, "0 amount"); // addToPosition: amount cannot be null

        _updatePool();
        address nftOwner = ERC721Upgradeable.ownerOf(tokenId);
        _harvestPosition(tokenId, nftOwner);

        StakingPosition storage position = _stakingPositions[tokenId];

        // we calculate the avg lock time:
        // lock_duration = (remainin_lock_time * staked_amount + amount_to_add * inital_lock_duration) / (staked_amount + amount_to_add)
        uint256 remainingLockTime = _remainingLockTime(position);
        uint256 avgDuration = (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration)
            / (position.amount + amountToAdd);

        position.startLockTime = _currentBlockTimestamp();
        position.lockDuration = avgDuration;

This function not only increases the amount of staked tokens for a user but it also adjusts the position.lockDuration of his stake by averaging out his previous stake and the amount he is adding now. It also sets the position.startLockTime to the current block.timestamp.
position.lockDuration and position.startLockTime are both used to determine if a user should be able to withdraw his staked tokens:

function _withdrawFromPosition(address nftOwner, uint256 tokenId, uint256 amountToWithdraw) internal {
     require(_unlockOperators.contains(nftOwner)
     || (position.startLockTime + position.lockDuration) <= _currentBlockTimestamp() || isUnlocked(),"locked");
     ...
}

By knowing all this we can come to the conclusion that an attacker can call addToPosition() using anyones tokenId (staking position) as input and prevent them from ever withdrawing by increasing their staked position by a small amount. This will increase the duration of their stake every time and they won't be able to withdraw.

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
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC721/ERC721Upgradeable.sol#L208-L219

Tool used

Manual Review

Recommendation

Adjust the _requireOnlyOperatorOrOwnerOf() function:

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

Duplicate of #378