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

Escrow Inconsistency Due to Unhandled Errors in Cancellation Process

lordofterra

Summary

The _cancelRequest function in the Starklane bridge contract, which is responsible for handling the cancellation process, lacks proper error handling for the _withdrawFromEscrow calls. This oversight can lead to inconsistent escrow states where some tokens may remain locked in escrow despite a cancellation attempt.

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L258C1-L266C6

Vulnerability Details

The _cancelRequest function iterates through token IDs and attempts to withdraw each from escrow. However, it does not handle potential errors from the _withdrawFromEscrow call. If a call fails silently, the function continues to the next token without any indication of the failure.

function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
_withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
}
}

Impact

1: Partial cancellations where some tokens are withdrawn from escrow while others remain locked.

2: Inconsistent escrow state, potentially leading to loss of user funds if tokens cannot be retrieved.

3: Difficulty in detecting which specific tokens failed to be withdrawn during the cancellation process.

4: Potential for subsequent operations to proceed with an incorrect understanding of the escrow state.

Tools Used

Manual review

Recommendations

1: Implement proper error handling for each _withdrawFromEscrow call:

function _cancelRequest(Request memory req) internal {
uint256 header = felt252.unwrap(req.header);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
address collectionL1 = req.collectionL1;
uint256[] memory failedWithdrawals = new uint256[](req.tokenIds.length);
uint256 failedCount = 0;
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool success = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
if (!success) {
failedWithdrawals[failedCount] = id;
failedCount++;
}
}
if (failedCount > 0) {
emit PartialCancellationCompleted(req.hash, failedWithdrawals[:failedCount]);
// Consider adding a mechanism to retry failed withdrawals
} else {
emit CancellationCompleted(req.hash);
}
}

2: Add a mechanism to track and retry failed withdrawals, ensuring all tokens can eventually be released from escrow.

3: Implement a state machine for cancellation requests to track their progress and final status.

4: Consider implementing a two-phase cancellation process where tokens are first marked for withdrawal and then actually withdrawn in a separate step, allowing for better error recovery.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.