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

Possibility of reverting when withdrawing Tokens from L1, if the receiver is an AA wallet

alqaqa

Vulnerability Details

When Bridging tokens from L2->L1, we provide the receiver address in L1, and after we made the tx on L2, gets proved by Starknet Protocol, the user can call L1Bridge::withdrawTokens() to receive his tokens in L1.

We extract the tokens Bridged from L2 to L1, and if that token was escrowed in L1Bridge we send it to the user from the L1Bridge to him, otherwise we mint it.

Bridge.sol#L201

function withdrawTokens( ... ) ... {
...
for (uint256 i = 0; i < req.tokenIds.length; i++) {
...
❌️ bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
...
}
...
}

When we transfer ERC721 token to the user we are using safeTransferFrom function, which checks if the receiver is a contract it calls onERC721Receiver() function.

Escrow.sol#L79

function _withdrawFromEscrow(... ) ... {
...
​
if (collectionType == CollectionType.ERC721) {
❌️ IERC721(collection).safeTransferFrom(from, to, id);
} else { ... }
...
}

The problem is that if the receiver is an Account Abstraction wallet, this function onERC721Receiver() is not a must to be presented. The implementation of AA Wallets didn't implement this interface nor it is implemented in the ERC4337. so if the receiver is an AA wallet the transaction will revert as it will not find that function and will revert.

This doesn't mean that the NFT will be lost. since AA wallets contains execute function that can call arbitraty calls, they can handle NFTs, so implementing that interface onERC721Receiver() is not a must.

Proof of Concept

  • UserA wants to Bridge NFTs from L2 to L1.

  • UserA wants to send his NFTs to his AA wallet on L1.

  • UserA initiated the tx and called L2Bridge::deposit_tokens().

  • When UserA wanted to call the L1Bridge::withdrawTokens() to withdraw his tokens.

  • tx reverted as the UserA AA wallet do not implement onERC721Receive().

  • NFTs will be locked in the Bridge, and lost forever.

Impact

if the receiver on L1 was an AA wallet NFTs may end up locked in the Bridge forever without having the apility to withdraw them.

Recommendations

Use transfer instead of safeTransfer when transferring NFTs.

diff --git a/apps/blockchain/ethereum/src/Escrow.sol b/apps/blockchain/ethereum/src/Escrow.sol
index c58bce9..47de782 100644
--- a/apps/blockchain/ethereum/src/Escrow.sol
+++ b/apps/blockchain/ethereum/src/Escrow.sol
@@ -76,7 +76,7 @@ contract StarklaneEscrow is Context {
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.
Updates

Lead Judging Commences

n0kto Lead Judge 9 months 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.