Summary
A user can have his L1 native NFT locked forever in the L1 bridge contract if he withdraws it and proceeds to redeposit using the ERC721 callback.
Vulnerability Details
In Escrow.sol:63, in `_withdrawFromEscrow`, an escrowed NFT is transferred to the user and the escrow mapping is updated to reflect that the contract no longer holds that token:
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 {
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
_escrow[collection][id] = address(0x0);
return true;
}
A user can have a legitimate use case of withdrawing tokens on L1, performing an action (such as claiming a collection exclusive airdrop) and immediately depositing back to Starknet.
However, if a user does that using the ERC721 callback, his token will be locked forever due to the line
_escrow[collection][id] = address(0x0);
which will run AFTER the callback and trick the contract into thinking it does not have the user's freshly deposited token.
POC
On mainnet, Alice deposits her L1 native token to Starknet.
On Staknet, Alice deposits back to mainnet.
On mainnet, Alice withdraws using her contract wallet.
Alice's contract callback redeposits to Starknet.
Alice's NFT is forever locked.
Alice's smart wallet interacts legitimately with the protocol, below is one possible implementation of her wallet:
pragma solidity ^0.8.0;
import "openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol";
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "../../src/IStarklane.sol";
@title SmartReceiver will interact correctly with the Bridge and have his token frozen forever.
*/
contract SmartReceiver is IERC721Receiver {
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata
) external returns (bytes4) {
address bridge = from;
address collection = msg.sender;
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
uint256[] memory ids = new uint256[](1);
ids[0] = tokenId;
IERC721(collection).setApprovalForAll(address(bridge), true);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
collection,
to,
ids,
false
);
return IERC721Receiver.onERC721Received.selector;
}
}
Below is a Forge test showing that she'll lose her beloved NFT:
function test_freeze_token_on_escrow() public{
address aliceBot = address(new SmartReceiver());
deal(aliceBot, 1 ether);
IERC721MintRangeFree(erc721C1).mintRangeFree(alice, 0, 10);
uint256[] memory ids = new uint256[](1);
uint256 tokenId = 0;
ids[0] = tokenId;
uint256 salt = 0x1;
snaddress to = Cairo.snaddressWrap(0x1);
vm.startPrank(alice);
IERC721(erc721C1).setApprovalForAll(address(bridge), true);
IStarklane(bridge).depositTokens{value: 30000}(
salt,
address(erc721C1),
to,
ids,
false
);
vm.stopPrank();
IStarklane(bridge).setL1L2CollectionMapping(address(erc721C1), Cairo.snaddressWrap(0x123), true);
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory req = buildRequestDeploy(header, tokenId, aliceBot);
req.collectionL1 = address(erc721C1);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
address collection = IStarklane(bridge).withdrawTokens(reqSerialized);
assertEq(collection, erc721C1);
assertEq(IERC721(erc721C1).ownerOf(tokenId), bridge);
felt252 header2 = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory req2 = buildRequestDeploy(header2, tokenId, alice);
req2.collectionL1 = address(erc721C1);
uint256[] memory reqSerialized2 = Protocol.requestSerialize(req2);
bytes32 msgHash2 = computeMessageHashFromL2(reqSerialized2);
uint256[] memory hashes2 = new uint256[](1);
hashes2[0] = uint256(msgHash2);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes2);
vm.expectRevert();
address collection2 = IStarklane(bridge).withdrawTokens(reqSerialized2);
}
Forge results:
├─ [25236] StarknetMessagingLocal::addMessageHashesFromL2([4967277853500676662229809734603431271824098072297973324452163283424245673967 [4.967e75]])
│ ├─ emit MessageHashesAddedFromL2(hashes: [4967277853500676662229809734603431271824098072297973324452163283424245673967 [4.967e75]])
│ └─ ← [Stop]
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [30631] ERC1967Proxy::withdrawTokens([257, 1, 0, 263400868551549723330807389252719309078400616203 [2.634e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1929, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 1, 0, 0, 0, 0, 0])
│ ├─ [30099] Starklane::withdrawTokens([257, 1, 0, 263400868551549723330807389252719309078400616203 [2.634e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1929, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 1, 0, 0, 0, 0, 0]) [delegatecall]
│ │ ├─ [10827] StarknetMessagingLocal::consumeMessageFromL2(1, [257, 1, 0, 263400868551549723330807389252719309078400616203 [2.634e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1929, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 1, 0, 0, 0, 0, 0])
│ │ │ ├─ emit ConsumedMessageToL1(fromAddress: 1, toAddress: ERC1967Proxy: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], payload: [257, 1, 0, 263400868551549723330807389252719309078400616203 [2.634e47], 291, 884601108998247062727812365178754106561321340343 [8.846e47], 1929, 0, 0, 0, 0, 0, 0, 0, 1094861636 [1.094e9], 4, 1, 0, 0, 0, 0, 0])
│ │ │ └─ ← [Return] 0x0afb612fa9ed90e49879c94bbdc9f79df5cbedf29ee31e07eeee539627ef6fef
│ │ ├─ [192] ERC721MintFree::mintFromBridge(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7, 0)
│ │ │ └─ ← [Revert] EvmError: Revert
│ │ └─ ← [Revert] EvmError: Revert
│ └─ ← [Revert] EvmError: Revert
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.32ms (1.34ms CPU time)
Impact
Users can lock their NFTs forever in the L1 bridge, by no fault of their own. Because users can lose NFTs worth several thousands of dollars, I'd say this is a High issue.
Tools Used
Foundry
Extra information
We described how this bug affects L1 native NFTs, but it will also affect Starknet native NFTs similarly:
On Starknet, Alice deposits her Starknet native NFT to mainnet.
On mainnet, withdraw and redeposit using the callback.
On Starknet, withdraw token and eventually redeposit to mainnet.
Mainnet contract will say NOT ESCROWED and try to mint once again, mint will fail because tokenId exists.
Result: L2 native token is locked BOTH on mainnet and Starknet.
Recommendations
Follow the CEI pattern and update the escrow before transferring the NFT, such that an eventual redeposit in callback will not be overwritten by a late update.
if (!_isEscrowed(collection, id)) {
return false;
}
_escrow[collection][id] = address(0x0);
