iamnmt - Incorrect ownership check in MlumStaking#addToPosition will cause locking user's funds more than a week as an attacker will call MlumStaking#addToPosition to user's position at the end of locking period

[sherlock-admin4]

iamnmt

Medium

Incorrect ownership check in MlumStaking#addToPosition will cause locking user's funds more than a week as an attacker will call MlumStaking#addToPosition to user's position at the end of locking period

Summary

Incorrect ownership check in MlumStaking#addToPosition will cause locking user's funds more than a week as the attacker will call MlumStaking#addToPosition at the end of locking period.

Vulnerability Detail

Since the check _requireOnlyOperatorOrOwnerOf always passes

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

An attacker can call MlumStaking#addToPosition to a user's position to increase its locking duration. The new locking duration is (remainingLockTime * position.amount + amountToAdd * position.initialLockDuration) / (position.amount + amountToAdd). At the end of a locking period remainingLockTime will tend to zero. To lock user's fund more than a week, the attacker will add to the position with amountToAdd such that

(amountToAdd * position.initialLockDuration) / (position.amount + amountToAdd) > 7 days
=> amountToAdd > (7 days * position.amount) / (position.initialLockDuration - 7 days) 

Required that position.initialLockDuration > 7 days. A position with large position.initialLockDuration will make it cheaper for the attacker to carry out this attack.

Impact

Locking user's funds in MlumStaking position more than a week.

Code Snippet

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

Tool used

Manual Review

Recommendation

Fix the MlumStaking#_requireOnlyOperatorOrOwnerOf function

src/MlumStaking.sol:140

    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(ERC721Upgradeable.ownerOf(tokenId), msg.sender, tokenId), "FORBIDDEN");
    }

Duplicate of #378