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

`withdrawTokens` function in `bridge.sol` is vulnerable to re-entrancy attacks

0xshoonya

Summary

The withdrawTokens function in the bridge.sol contract is vulnerable to re-entrancy attacks. This vulnerability arises from the order of operations in the _withdrawFromEscrow function, where the token transfer occurs before the escrow state is updated.

Vulnerability Details

When a user calls withdrawTokens on bridge contract to withdraw tokens received from L2, it verifies for each token ID in the request payload whether it is escrowed or not by calling the _withdrawFromEscrow function:

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
// ...
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
@> bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
​
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
​
return collectionL1;
}

Inside _withdrawFromEscrow, if the token is found in the escrow (checked by _isEscrowed(collection, id)), the token is transferred using safeTransferFrom:

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;
}

The critical issue is that the escrow state is updated after the token transfer.

The safeTransferFrom calls invoke the onERC721Received function in the to address (i.e., the recipient contract). If the recipient contract is malicious, it could recursively call withdrawTokens before the escrow state (_escrow[collection][id] = address(0x0);) is updated.

Impact

Bridge contract on L1 can be drained of its escrowed tokens.

Tools Used

Manual Review

Recommendations

Update the _escrow state before calling safeTransferFrom:

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
address from = address(this);
​
+ _escrow[collection][id] = address(0x0);
​
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
Invalidated
Reason: Lack of quality
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.