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

Potential Unauthorized Request Cancellation in Starklane Bridge

mansa11

Summary

The cancelRequest function in the Starklane bridge contract allows any user to cancel a deposit request, potentially leading to unauthorized cancellations and loss of user funds.

Vulnerability Details

The cancelRequest function (lines 245-258) is designed to cancel a pending L1 to L2 message and return the deposited tokens to the original owner. However, the function lacks proper access control. It doesn't verify that the caller (msg.sender) is the original depositor or an authorized party, hence a malicious actor just have to get a nonce and setup a payload from the LogMessageToL2 event looged in the StarknetMessaging::sendMessageToL2 before constructing a payload that will be used to the request.

The relevant code snippet

function cancelRequest( //@audit anyone can cancel another users deposit before it gets sent on the l2. should add a logic that checks if address ownerL1 is msg.sender
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);
}

The event log that an attacker needs to read from the memepool is gotten from here;

function sendMessageToL2(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload
) external payable override returns (bytes32, uint256) {
require(msg.value > 0, "L1_MSG_FEE_MUST_BE_GREATER_THAN_0");
require(msg.value <= getMaxL1MsgFee(), "MAX_L1_MSG_FEE_EXCEEDED");
uint256 nonce = l1ToL2MessageNonce();
NamedStorage.setUintValue(L1L2_MESSAGE_NONCE_TAG, nonce + 1);
emit LogMessageToL2(msg.sender, toAddress, selector, payload, nonce, msg.value); //@audit this is what event the attacker will read from the memepool including the nonce. the attacker only needs the nonce and the payload
bytes32 msgHash = getL1ToL2MsgHash(toAddress, selector, payload, nonce);
// Note that the inclusion of the unique nonce in the message hash implies that
// l1ToL2Messages()[msgHash] was not accessed before.
l1ToL2Messages()[msgHash] = msg.value + 1;
return (msgHash, nonce);
}

Impact

The vulnerability could lead to:

  1. Unauthorized cancellation of legitimate deposit requests

  2. Potential loss or temporary lock of user funds

  3. Disruption of the bridge's normal operation

  4. Loss of user trust in the bridge system

  5. The severity is high due to the potential for direct financial loss to users.

PROOF OF CONCEPT

function test_cancelRequestPOC() public {
uint256[] memory ids = new uint256[](2);
ids[0] = 0;
ids[1] = 9;
​
(uint256 nonce, uint256[] memory payload) = setupCancelRequest(alice, ids);
assert(IERC721(erc721C1).ownerOf(ids[0]) != alice);
assert(IERC721(erc721C1).ownerOf(ids[0]) != alice);
​
Request memory req = Protocol.requestDeserialize(payload, 0);
​
vm.expectEmit(true, false, false, false, bridge);
emit CancelRequestStarted(req.hash, 42);
IStarklane(bridge).startRequestCancellation(payload, nonce);
​
vm.expectEmit(true, false, false, false, bridge);
emit CancelRequestCompleted(req.hash, 42);
//
vm.prank(bob); //@audit anyone can cancel another users request he just have to get a nonce and setup a payload from the LogMessageToL2 event
IStarklane(bridge).cancelRequest(payload, nonce);
​
assert(IERC721(erc721C1).ownerOf(ids[0]) == alice);
assert(IERC721(erc721C1).ownerOf(ids[1]) == alice);
}

Tools Used

Manual code review

Recommendations

Implement access control in the cancelRequest function:

function cancelRequest(uint256[] memory payload,uint256 nonce) external {
Request memory req = Protocol.requestDeserialize(payload, 0);
require(msg.sender == req.ownerL1, "Unauthorized cancellation");
// Rest of the function remains the same
}
Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

invalid-cancel-callable-by-anyone

The process to cancel a message is detailed here: https://docs.starknet.io/architecture-and-concepts/network-architecture/messaging-mechanism/#l2-l1_message_cancellation Since `startRequestCancellation` has the `onlyOwner`, only the owner can begin that process.

Support

FAQs

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