ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Tokens irrecoverable by owner on L1 if not an `ERC721` receiver

0xshoonya

Summary

The current implementation of the withdrawTokens function on L1 Bridge introduces a crucial security risk where ERC721 tokens could become permanently locked within the escrow contract if the receiving address does not support the ERC721TokenReceiver interface.

Vulnerability Details

When a user calls withdrawTokens to withdraw ERC721 tokens received from L2, the function attempts to transfer tokens from the escrow.

function withdrawTokens(
uint256[] calldata request
) external payable returns (address)
{
...snip...
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
@> bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
​
...snip...
}

The vulnerability arises in the _withdrawFromEscrow function, which handles the transfer of tokens from the escrow contract back to the user's L1 address (req.ownerL1):

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
@> IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
_escrow[collection][id] = address(0x0);
​
return true;
}

The escrow contract attempts to transfer the tokens using safeTransferFrom. The safeTransferFrom method ensures that the recipient contract implements the ERC721TokenReceiver interface. If the recipient (req.ownerL1) is a contract that does not support receiving ERC721 tokens (i.e., it does not implement ERC721TokenReceiver), the transfer will revert. This leads to the token being permanently locked in the escrow contract.

Impact

Users who have their L1 receiving address set to a contract that doesn't support ERC721TokenReceiver will be unable to complete their withdrawals and their tokens will be permanently locked in the escrow contract.

Tools Used

Manual Review

Recommendations

Consider using transferFrom() instead of safeTransferFrom() when transferring the tokens back to the owner in _withdrawFromEscrow. This will allow the transfer to succeed even if the owner on L1 is not an ERC721 receiver.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-safeTransferFrom-to-no-onERC721Received-will-revert

Impact: High, NFT will be stuck in L2 bridge. Likelyhood: Very low, sending NFT to a contract not implementing that function would almost be a user error.

Appeal created

nirohgo Auditor
about 1 year ago
n0kto Lead Judge
about 1 year ago
n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-safeTransferFrom-to-no-onERC721Received-will-revert

Impact: High, NFT will be stuck in L2 bridge. Likelyhood: Very low, sending NFT to a contract not implementing that function would almost be a user error.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.