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

Should follow CEI pattern in Starklane's _withdrawFromEscrow Function

fakemonkgin

Summary

The _withdrawFromEscrow function within Starklane contract is responsible for withdrawing tokens from escrow and transferring them to a designated address. However, the function currently does not follow the Checks-Effects-Interactions (CEI) pattern, which is critical for preventing reentrancy attacks.

Vulnerability Details

The _withdrawFromEscrow function includes external calls to ERC721 or ERC1155 contracts through the safeTransferFrom method before updating the internal escrow state (_escrow[collection][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); @audit should follow CEI
​
return true;
}

Impact

This order of operations exposes the function to reentrancy attacks, as it violates the CEI pattern.

Tools Used

Manual Review

Recommendations

To mitigate the reentrancy risk, the _withdrawFromEscrow function should be modified to adhere to the CEI pattern or apply a nonReentrant modifier (e.g., from OpenZeppelin’s ReentrancyGuard) to ensure the function cannot be re-entered during execution.

function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
// Update the escrow mapping before making the external call
_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, "");
}
​
return true;
}
​
Updates

Lead Judging Commences

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