Summary

The return value of transfer_from is not checked, this can cause if one of the transferred NFTs fails, the transfer loop will continue and the receiver will get less NFT than they should

Vulnerability Details

In the bridge.cairo contract, there are two functions that use transfer_from to transfer NFTs. The first function is withdraw_auto_from_l1() and the second is escrow_deposit_tokens().

a. withdraw_auto_from_l1()

The purpose of this function is to transfer NFT from L1 to owner L2. And this is done with a loop according to the number of NFTs available. As can be seen from the code above, if the NFT is in the escrow contract, the transfer will be carried out. The main problem here is that if one of the transfers fails, the loop process will continue without reverting until the last NFT amount. This can cause the NFT received by the L2 owner to not match the NFT sent from L1 because there is no error handling in this function.

b. escrow_deposit_tokens()

The main problem with this function is the same as the function above. What differs is the purpose of this function. The purpose of this function is to deposit NFTs into an escrow contract. If one of the transfers fails in the loop, then the transfer process will continue until the last NFT and cause the escrow contract to receive less NFT than it sent.

Impact

The receiver will get less NFT than they should

Tools Used

Manual Review

Recommended Mitigation

Consider adding error handling in the loop

1. withdraw_auto_from_l1()

   if is_escrowed {

   let success = IERC721Dispatcher { contract_address: collection_l2 }

   .transfer_from(from, to, token_id);

   assert(success, 'transfer fail');

   ​

2. escrow_deposit_tokens()

   let success = erc721.transfer_from(from, to, token_id);

   assert(success, 'transfer fail');

   ​