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

Reentrancy in the Starklane::withdrawTokens function, can cause the protocol lose assets

emma77

Summary

The Starklane::withdrawTokens function implements the code line bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
The _withdrawFromEscrow function makes calls before update the state variables.

Vulnerability Details

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
// Header is always the first uint256 of the serialized request.
uint256 header = request[0];
​
// Any error or permission fail in the message consumption will cause a revert.
// After message being consumed, it is considered legit and tokens can be withdrawn.
if (Protocol.canUseWithdrawAuto(header)) {
// 2024-03-19: disabled autoWithdraw after audit report
// _consumeMessageAutoWithdraw(_starklaneL2Address, request);
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
​
Request memory req = Protocol.requestDeserialize(request, 0);
​
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
​
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
​
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
// update whitelist if needed
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
@> bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id); // @audit Reentrancy
​
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); // @audit Reentrancy
}
}
​
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
​
return collectionL1;
}

Impact

Reentrancy attack that can lead to assets been lost by the protocol

Tools Used

manual review

Recommendations

Implement the CEI pattern or
Reentrancy Guards in the

StarklaneEscrow::_withdrawFromEscrow

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
+ _escrow[collection][id] = address(0x0);
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
IERC721(collection).safeTransferFrom(from, to, id); //@audit --> Reentrancy attack?
// 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: Non-acceptable severity
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.