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

Transfering escrowed NFTs in Ethereum is safe whereas minting new ones not

draiakoo

Summary

Transfering escrowed NFTs in Ethereum is safe whereas minting new ones not

Vulnerability Details

In the Starknet bridge implementation, when a token is either escrowed or it has to be minted, it does not check for the ERC721 acceptance in case it is a contract.

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
...
let is_escrowed = !self.escrow.read((collection_l2, token_id)).is_zero();
​
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
} else {
if (req.uris.len() != 0) {
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}
​
i += 1;
};
​
self.emit(WithdrawRequestCompleted {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}

Internally, the transfer_from method does not check if the contract can handle NFTs and mint_from_bridge, mint_from_bridge_uri neither do.
On the other hand, in the Ethereum implementation, the escrowed transfers are 'safe' which checks for the acceptance of NFTs in case the receiver is a contract but minting new ones don't.

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
...
​
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);
}
}
​
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
​
return collectionL1;
}
​
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;
}

This only method being safe can make incompatible for some contracts, because a contract will be able to withdraw tokens that are newly minted without implementing the ERC721 receiver interface, but will unable to withdraw tokens that already exist.

Impact

Medium
Imagine the following situation:

  1. A user con Starknet sends an NFT to an Ethereum contract

  2. The contract does not implement the ERC721Receiver interface but can still withdraw the NFT

  3. It sends it back to Starknet

  4. The user on Starknet sends back to the Ethereum contract

  5. The NFT gets stuck forever because the contract does not implement the interface to receive the NFT

Tools Used

Manual review

Recommendations

Making all the transfering/minting methods similiar with the acceptance method will lead to a more consistent protocol. My recommendation is to set all the methods to not check for acceptance because there is only a single situation where it checks for acceptance, all the other ones don't.

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);
+ IERC721(collection).transferFrom(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;
}
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.

Support

FAQs

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

Give us feedback!