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

Bridge Withdraw Implementation Results in Stuck Tokens On `IERC721Receiver`s

auditweiler

Summary

Withdrawing from the Bridge is incompatible with contracts that implement IERC721Receiver.

Vulnerability Details

The relevant withdrawTokens implementation is listed below:

for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}

The logic works as follows.

If the token was in circulation when bridged (i.e. purchased from a marketplace), it can be withdrawn from the escrow via _withdrawFromEscrow:

if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id); /// @audit does_use_safeTransfer
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}

Conversely, if the token is one of the proxy implementations used to represent a collection item bridged from Starknet, the corresponding token is newly minted via mintFromBridge:

function mintFromBridge(address to, uint256 id)
public
onlyOwner {
_mint(to, id); /// @audit does_not_use_safeMint
}

Notice that these transfers are implemented inconsistently.

When transferring a token to a contract receiver during _withdrawFromEscrow, we correctly invoke onERC721Received to prevent tokens from being sent to receiver contracts which fail to acknowledge the inbound transfer of NFTs.

However, when dealing with bridge tokens, we use _mint instead, meaning that we do risk transferring tokens to incompatible receivers depending on the underlying token type:

/**
* @dev Mints `tokenId` and transfers it to `to`.
*
* WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible
*
* Requirements:
*
* - `tokenId` must not exist.
* - `to` cannot be the zero address.
*
* Emits a {Transfer} event.
*/
function _mint(address to, uint256 tokenId) internal {
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address previousOwner = _update(to, tokenId, address(0));
if (previousOwner != address(0)) {
revert ERC721InvalidSender(address(0));
}
}

This means any contracts which define the onERC721Received callback as a critical component of the transaction lifecycle are compatible with tokens that originate from the market, but incompatible with those specifically generated by Ark.

Impact

Stuck bridgeable tokens on destination contracts that demand communication via onERC721Received.

Tools Used

Manual Review

Recommendations

Use safeMint whilst minting bridgeable tokens.

Note, following this recommendation may result in unintentional re-entrancy. Consider adding a nonReentrant modifier to all untrusted external parents of invocations to mintFromBridge.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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