sherlock-audit / 2023-03-optimism-judging

7 stars 0 forks source link

Bauer - If user uses an NFT that can be paused,the NFT may be frozen in the contract #32

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Bauer

medium

If user uses an NFT that can be paused,the NFT may be frozen in the contract

Summary

If user uses an NFT that can be paused, and it is discovered by a bad actor during the pause period, it may result in the user's NFT being frozen in the contract.

Vulnerability Detail

The protocol allows user to bridge an NFT to some recipient's account from L2 to L1.On the L1 side, after challenge period and validation elapsed, the protocol will transfer NFT to user during relaying the message. However, some NFTs like CryptoKitty and CryptoFighter may be paused, and if a user uses these types of NFTs and they are discovered by bad actors during the pause, bad actors can call the finalizeWithdrawalTransaction() function to make the transaction fail ,permanently freeze the user's NFT in the contract. Crypto-figher NFT: https://etherscan.io/address/0x87d598064c736dd0C712D329aFCFAA0Ccc1921A1#code#L873

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    public
    whenNotPaused
{

Crypto-kitty NFT: https://etherscan.io/address/0x06012c8cf97BEaD5deAe237070F9587f8E7A266d#code#L615

function transferFrom(
    address _from,
    address _to,
    uint256 _tokenId
)
    external
    whenNotPaused
{

1.Alice uses NFT like CryptoKitty and CryptoFighter to bridge. 2.Bob notices that Alice is bridging these bytes of NFT from L2 to L1. Her transaction is proved but she is waiting for the challenge period to be finished to finalize it. 3.Coincidentally, this NFT has been paused. Then, after the challenge period is passed, Bob calls the finalizeWithdrawalTransaction() using Alice's transaction information as a parameter. 4.Due to the NFT is paused, the call will be unsuccessful. Since, the failed call is not handled during finalizing the message, the transaction will be finished without any error.By doing so, Alice's transaction is flagged as finalized, but in reality it was not because the NFT is paused. So, Alice loses her NFT.

   require(
            finalizedWithdrawals[withdrawalHash] == false,
            "OptimismPortal: withdrawal has already been finalized"
        );

        // Mark the withdrawal as finalized so it can't be replayed.
        finalizedWithdrawals[withdrawalHash] = true;

        // Set the l2Sender so contracts know who triggered this withdrawal on L2.
        l2Sender = _tx.sender;

        // Trigger the call to the target contract. We use a custom low level method
        // SafeCall.callWithMinGas to ensure two key properties
        //   1. Target contracts cannot force this call to run out of gas by returning a very large
        //      amount of data (and this is OK because we don't care about the returndata here).
        //   2. The amount of gas provided to the call to the target contract is at least the gas
        //      limit specified by the user. If there is not enough gas in the callframe to
        //      accomplish this, `callWithMinGas` will revert.
        // Additionally, if there is not enough gas remaining to complete the execution after the
        // call returns, this function will revert.
        bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

Impact

User will ose NFT

Code Snippet

https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L383-L397 https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/L1ERC721Bridge.sol#L68

Tool used

Manual Review

Recommendation

Blacklist these types of NFTs

GalloDaSballo commented 1 year ago

Technically correct, but I believe OOS because the Sponsor only supports token with regular behaviour

https://app.sherlock.xyz/audits/contests/63

hrishibhat commented 1 year ago

Sponsor comment: Blocking specific ERC721 contracts is a design decision and not in the audit scope.