Vulnerability Details

When bridging NFTs from L1 -> L2, there is an option to cancel messages, using Starknet Messaging Protocol if the sequencer did not process it (because of the lack of gas or something). To do this we call startL1ToL2MessageCancellation and after some period we call cancelL1ToL2Message to cancel it.

The protocol uses this to let the user who Escrowed his NFT(s) can withdraw them back if his message was not processed by the sequencer, and when canceling the message we are returning the NFTs back to him.

Bridge.sol#L264

As we can see when withdrawing we are sending NFT(s) to the req.ownerL1, but is req.ownerL1 the owner of the NFT?

When calling L1Bridge::depositTokens, we are signing req.ownerL1 to msg.sender.

Bridge.sol#L117

Then we transfer the NFT tokens from him (msg.sender) to the Bridge.

Escrow.sol#L38-L40

The problem is that ERC721 token supports two types of approvals full approvals (isApprovedForAll()), and single approvals (getApproved()). So the sender of the TX can be another address that is approved to use these NFTs and not the actual owner.

Now if the caller was an approved address for NFT(s) to be Bridge, when we cancel it this will result in transfering NFT(s) to the Sender of the TX (approval) and not to the actual owner.

The problem can be even more critical as the sender may be a smart contract approved to use user NFT(s), like an NFT Manager. So when trying to cancel the message the tx will either revert because of using safeTransferFrom and the contract may not have onERC721Received function (As it is not intended to receive NFTs it just transfers on behalf of users), or success by sending the NFT to that contract (and there may be no method to get them back).

So in brief, the NFTs will end up going to a different address than the original owner of them.

Proof of Concept

• UserA has an NFT Manager contract which he uses to transfer his NFTs.

• He Bridged his NFT(s) using that NFT Manager.

• The message wasn't processed by the sequencer.

• UserA requested to cancel it from the protocol.

• Protocol Devs started cancelation by calling startL1ToL2MessageCancellation()

• Time Passes...

• UserA called cancelL1ToL2Message() to cancel the message and take his NFT(s) back.

• NFT(s) is going to be sent to the NFT Manager and not the original owner.

• Transaction reverted as NFT manager is not implementing onERC721Received().

Impact

NFT(s) will get transferred to the incorrect owner or even lost forever.

Tools Used

Manual Review

Recommendations

Make the escrow mapping store the original owner of the NFT instead of the sender, and when withdrawing them send them to the escrowed addresses.

NOTE:This behavior should only be used when canceling messages. If it is L2 -> L1 message and we are withdrawing from Escrow, we should give them to the ownerL1 directly.