NFTBridge
60,000 USDC
View results
Submission Details
Severity: high
Invalid

If the contract owner is unavailable users will lose assets due to failed message consumption.

Summary

The message cancellation feature is used to cancel failed transaction to prevent locking of assets forever.

However cancellation can only be invoked by the contract's owner which is not feasible as many users might have briged and due to a bug in the bridge the consumption fails, Now all users will have to wait on the owner to cancel the request which will lead to loss of assets for some.

The use of the onlyOwnermodifier here is to prevent cancellations request spamming which is already prevented with a five day wait before users can finalize cancellations. The modifier is not needed in this case for flexibility reasons.

Vulnerability Details

Consider that Alice sends an L1 asset to a Starknet bridge to transfer it to L2, which generates the corresponding L1→L2 message. Now, consider that the L2 message consumption doesn’t function, which might happen due to a bug in the dApp’s Cairo contract. This bug could result in Alice losing custody of their asset forever.

The steps in this flow are as follows:

The contract owner calls `startRequestCancellation` which invokes `startL1ToL2MessageCancellation` function in the Starknet Core Contract.

function startRequestCancellation(
uint256\[] memory payload,
uint256 nonce
) external onlyOwner {
IStarknetMessaging(\_starknetCoreAddress).startL1ToL2MessageCancellation(
snaddress.unwrap(\_starklaneL2Address),
felt252.unwrap(\_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
emit CancelRequestStarted(req.hash, block.timestamp);
}

The user who bridged waits five days until she can finalize the cancellation.

The user calls the `cancelL1ToL2Message` function.

function cancelRequest(
uint256\[] memory payload,
uint256 nonce
) external {
IStarknetMessaging(\_starknetCoreAddress).cancelL1ToL2Message(
snaddress.unwrap(\_starklaneL2Address),
felt252.unwrap(\_starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
\_cancelRequest(req);
emit CancelRequestCompleted(req.hash, block.timestamp);
}

As stated on the starknet docs, to prevent against DoS attacks by repeatedly sending and cancelling:

```

To mitigate this risk, the contract that initiated the L1→L2 message can cancel it by declaring the intent to cancel, waiting five days, and then completing the cancellation. This delay protects the sequencer from a DoS attack in the form of repeatedly sending and canceling a message before it is included in L1, rendering the L2 block which contains the activation of the corresponding L1 handler invalid.

```

The use of onlyOwner here will prevent certain users from cancelling.

Impact

Loss of assets if owner doesn't start cancellations on behalf of users

Tools Used

Manual Review

Recommendations

Remove the `onlyOwner` modifier.

Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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