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

Attackers Can Create Irredeemable Bridge Tokens

auditweiler

Summary

Since neither depositTokens or withdrawTokens are protected from re-entrancy, it is possible for an attacker to manipulate the protocol into the belief that a bridged token has not been escrowed. This can be exploited to create tokens which cannot be unbridged.

Vulnerability Details

In this scenario, let's an attacker with a collectionL1 token that originates on Ethereum L1 (i.e. it is an Ethereum-native ERC-721 currently in circulation and not an IERC721Bridgeable).

  1. First, the attacker calls depositTokens and bridges the token onto Starknet, and then withdraws proxy token from the Starknet bridge.

  2. Following this, the attacker then invokes escrow_deposit_tokens to initialize the process of unbridging the token back on the L1. Importantly, the attacker specifies their own malicious contract address as the req.ownerL1 receiver.

  3. On Ethereum, the attacker calls withdrawTokens, which underlyingly usessafeTransferFrom to transfer tokens, consequently invoking the onERC721Received callback on the attacker's malicious destination contract.

  4. Since the L1 Bridge contract does not protect from re-entrancy, it is possible for the malicious contract to make a re-entrant invocation of depositTokens to re-deposit the token whilst the withdrawal from the escrow is still ongoing. Crucially, because the _withdrawFromEscrow flow does not adhere to the Checks, Effects, Interactions pattern, we can see that after the attacker's deposit operation concludes, the Escrow contract makes the invalid assertion that _escrow[collection][id] = address(0x0), even though this token has indeed been escrowed.

  5. After this, the token is bridged back to Starknet.

At this point in the attack, the attacker has successfully bridged an Ethereum-native token to Starknet, but the Bridge on Ethereum believes that the native token it holds has not been escrowed:

if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id); /// @audit invokes_malicious_callback
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
_escrow[collection][id] = address(0x0); /// @audit token_is_not_believed_to_be_escrowed

It is now impossible for the token to be unbridged back on Ethereum Mainnet, since invocations to withdrawTokens will correctly identify the correct mainnet address of the bridged token via _verifyRequestAddresses, but the bridged token will fail to return true for isEscrowed:

/**
@notice Verifies if the given token is in escrow.
@param collection Token collection address.
@param id Token id.
@return True if the token is in escrow, false otherwise.
*/
function _isEscrowed(
address collection,
uint256 id
)
internal
view
returns (bool)
{
return _escrow[collection][id] > address(0x0); /// @audit this_returns_false
}
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
/// ...

Consequently, the Bridge contract will attempt to mintFromBridge, resulting in revert:

for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
/// @audit This call erroneously returns false for the token.
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
/// @audit The bridge attempts to invoke `mintFromBridge` on
/// @audit a third-party contract, and subsequently reverts:
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);
}
}

This makes it impossible to remove the token from the bridge.

Impact

It is possible for malicious users to prevent tokens from being ever bridged back from Ethereum Mainnet.

It is also plausable that the machinations of complex dependent protocols may inadvertently lock up tokens on the Bridge if deposits were to be invoked re-entrantly, for example, based upon the dictation of an order book at a cross-chain hub.

Tools Used

Manual Review

Recommendations

Follow the CEI pattern:

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
+ /// @notice Mark the token as no longer in escrow
+ /// @notice before handing control back to the caller.
+ _escrow[collection][id] = address(0x0);
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;
}
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year 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.