sherlock-audit / 2024-06-magicsea-judging

8 stars 5 forks source link

scammed - If owner of the lsNFT is USDC blacklisted he won't be able to call addToPosition() #516

Closed sherlock-admin4 closed 3 months ago

sherlock-admin4 commented 4 months ago

scammed

Medium

If owner of the lsNFT is USDC blacklisted he won't be able to call addToPosition()

Summary

addToPossition, withdrawFromPosition, renewLockPosition and extendLockPosition cannot be called if the owner of the lsNFT gets blacklisted on the rewardToken (USDC).

Vulnerability Detail

All these functions harvest the reward up to that point for the given NFT. But since the reward token is USDC and the owner of the NFT as an address can be blacklisted, then he cannot receive transfers from USDC, because of this different address must be passed as receiver of the rewards, as in harvestPositionTo.

[MlumStaking.sol#L456-L463](https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L456-L463)

function harvestPositionTo(uint256 tokenId, address to) external override nonReentrant {
    _requireOnlyApprovedOrOwnerOf(tokenId);
    // legacy: require(ERC721.ownerOf(tokenId).isContract(), "FORBIDDEN");

    _updatePool();
    _harvestPosition(tokenId, to);
    _updateBoostMultiplierInfoAndRewardDebt(_stakingPositions[tokenId]);
}

But all the functions mentioned above use harvestPosition and always pass the NFT owner as USDC receiver, which will revert if the user gets blacklisted and thus will block the usage of the NFT because he can't deposit, withdraw and lock.

[MlumStaking.sol#L619-L650](https://github.com/sherlock-audit/2024-06-magicsea/blob/42e799446595c542eff9519353d3becc50cdba63/magicsea-staking/src/MlumStaking.sol#L619-L650)

function _withdrawFromPosition(address nftOwner, uint256 tokenId, uint256 amountToWithdraw) internal {
    require(amountToWithdraw > 0, "null");
    // withdrawFromPosition: amount cannot be null

    StakingPosition storage position = _stakingPositions[tokenId];
    require(
        _unlockOperators.contains(nftOwner)
            || (position.startLockTime + position.lockDuration) <= _currentBlockTimestamp() || isUnlocked(),
        "locked"
    );
    // withdrawFromPosition: invalid amount
    require(position.amount >= amountToWithdraw, "invalid");

    _harvestPosition(tokenId, nftOwner); // <---------------- @audit should pass to instead of nftOwner 

    // update position
    position.amount = position.amount - amountToWithdraw;

    // update total lp supply
    _stakedSupply = _stakedSupply - amountToWithdraw;

    if (position.amount == 0) {
        // destroy if now empty
        _stakedSupplyWithMultiplier = _stakedSupplyWithMultiplier - position.amountWithMultiplier;
        _destroyPosition(tokenId);
    } else {
        _updateBoostMultiplierInfoAndRewardDebt(position);
    }

    emit WithdrawFromPosition(tokenId, amountToWithdraw);
    stakedToken.safeTransfer(nftOwner, amountToWithdraw);
}

Impact

The NFT will become unusable if the owner gets blacklisted for USDC.

Code Snippet

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

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

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

Tool used

Manual Review

Recommendation

harvestPositionTo is implemented just for this case. Replace harvestPosition with harvestPositionTo in all these functions and let the caller pass to address.

0xSmartContract commented 3 months ago

The issue is invalid because the problem arises from an external decision (i.e., the blacklisting by USDT or USDC), which is not under the control of the protocol itself. According to the Sherlock Criteria, blacklisting and related impacts are not considered valid issues for the contest. The focus should be on vulnerabilities within the protocol's own smart contracts and their design, rather than external actions by third-party tokens.