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

Malicious actor can drain escrowed tokens through reentrancy (`Starklane::withdrawTokens`)

4gontuk

Summary

Vulnerability Detail

The Starklane contract, which serves as a bridge between Ethereum and Starknet, contains a critical vulnerability in its withdrawTokens() function. This function interacts with the StarklaneEscrow contract's _withdrawFromEscrow() function to facilitate token withdrawals. The issue lies in the order of operations within _withdrawFromEscrow(), where the token transfer occurs before the escrow state is updated, creating a reentrancy vulnerability.

The withdrawTokens() function in Starklane is responsible for processing withdrawal requests from Starknet. It deserializes the request, verifies collection addresses, and calls _withdrawFromEscrow() for each token to be withdrawn. The _withdrawFromEscrow() function in StarklaneEscrow performs the following steps:

  1. Checks if the token is in escrow

  2. Transfers the token using safeTransferFrom()

  3. Updates the escrow state

The main flaw is that the state update occurs after the token transfer. This sequence allows a malicious contract to reenter the withdrawTokens() function before the escrow state is updated, potentially withdrawing the same token multiple times.

Relevant code snippets:

// In Starklane.sol
function withdrawTokens(uint256[] calldata request) external payable returns (address) {
// ... (request processing)
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
// ...
}
// ...
}
​
// In StarklaneEscrow.sol
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 {
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
_escrow[collection][id] = address(0x0);
return true;
}

Impact

An attacker can exploit this reentrancy vulnerability to drain multiple tokens from the escrow by repeatedly calling withdrawTokens() before the state is updated. This can lead to a significant loss of tokens from the escrow, undermining the security and integrity of the bridge.

Proof of Concept

  1. Attacker deploys a malicious contract with a fallback function that calls Starklane.withdrawTokens().

  2. Attacker initiates a legitimate withdrawal through Starklane.withdrawTokens().

  3. StarklaneEscrow._withdrawFromEscrow() is called and transfers the token to the attacker's contract.

  4. The attacker's fallback function is triggered, calling withdrawTokens() again before the escrow state is updated.

  5. Steps 3-4 repeat until the attacker has drained desired tokens from the escrow.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, implement the following changes:

  1. Update the escrow state before transferring tokens in _withdrawFromEscrow().

  2. Implement OpenZeppelin's ReentrancyGuard in both Starklane and StarklaneEscrow contracts.

Here's the recommended fix for StarklaneEscrow._withdrawFromEscrow():

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract StarklaneEscrow is Context {
+ contract StarklaneEscrow is Context, ReentrancyGuard {
// ...
​
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
+ nonReentrant
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);
} else {
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
- _escrow[collection][id] = address(0x0);
​
return true;
}
// ...
}

Also, add the nonReentrant modifier to Starklane.withdrawTokens():

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, CollectionManager {
+ contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, CollectionManager, ReentrancyGuard {
// ...
​
function withdrawTokens(
uint256[] calldata request
)
external
payable
+ nonReentrant
returns (address)
{
// ... (existing code)
}
// ...
}
Updates

Lead Judging Commences

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