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

Sudden Change of Parameters Like `_starklaneL2Address` and `_starklaneL2Selector` Will Cause The Request Can't Be Cancelled

jesjupyter

Summary

The functions setStarklaneL2Address and setStarklaneL2Selector allow the owner to modify critical state variables such as _starklaneL2Address and _starklaneL2Selector. These variables are essential for generating the msgHash in the StarknetMessaging::getL1ToL2MsgHash function, which tracks the state of messages between L1 and L2. However, if these variables are altered after a request is made, it could change the hash generated for the same request, leading to problems during the cancellation process. Specifically, the functions startRequestCancellation and cancelRequest would fail to produce the correct hash, potentially leading to an inability to cancel requests and causing tokens to be locked in the bridge.

This issue shares similarities with maintaining consistency between setups across chains, which could also result in problems with token withdrawals.

Vulnerability Details

The setStarklaneL2Address and setStarklaneL2Selector functions allow the contract’s owner to change the _starklaneL2Address and _starklaneL2Selector variables.

/**
@notice Sets Starklane L2 address.
​
@param l2Address Starklane L2 address.
*/
function setStarklaneL2Address(
uint256 l2Address
)
public
onlyOwner
{
_starklaneL2Address = Cairo.snaddressWrap(l2Address);
}
​
/**
@notice Sets Starklane L2 selector of Starklane L2 contract to be
called when a message arrives into Starknet.
​
@param l2Selector Starklane L2 selector.
*/
function setStarklaneL2Selector(
uint256 l2Selector
)
public
onlyOwner
{
_starklaneL2Selector = Cairo.felt252Wrap(l2Selector);
}

These variables are used in several critical functions, including depositTokens, withdrawTokens, startRequestCancellation, and cancelRequest.

// `depositTokens`
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
// `withdrawTokens`
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
​
// `startRequestCancellation`
IStarknetMessaging(_starknetCoreAddress).startL1ToL2MessageCancellation(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);
​
// `cancelRequest`
IStarknetMessaging(_starknetCoreAddress).cancelL1ToL2Message(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload,
nonce
);

These variables play a crucial role in generating the message hash using the StarknetMessaging::getL1ToL2MsgHash function:

function getL1ToL2MsgHash(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload,
uint256 nonce
) internal view returns (bytes32) {
return
keccak256(
abi.encodePacked(
uint256(uint160(msg.sender)),
@=> toAddress,
nonce,
@=> selector,
payload.length,
payload
)
);
}

When _starklaneL2Address or _starklaneL2Selector is changed, the generated msgHash will differ from the original one for the same request. This discrepancy could cause failures in the startRequestCancellation and cancelRequest functions, as they rely on the correct hash to track the state of messages in relevant functions in StarknetMessaging.

function startL1ToL2MessageCancellation(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload,
uint256 nonce
) external override returns (bytes32) {
emit MessageToL2CancellationStarted(msg.sender, toAddress, selector, payload, nonce);
@=》 bytes32 msgHash = getL1ToL2MsgHash(toAddress, selector, payload, nonce);
uint256 msgFeePlusOne = l1ToL2Messages()[msgHash];
@=》 require(msgFeePlusOne > 0, "NO_MESSAGE_TO_CANCEL");
l1ToL2MessageCancellations()[msgHash] = block.timestamp;
return msgHash;
}

Consequently, users might find themselves unable to cancel their requests, leading to the locking of their tokens within the bridge.

This issue shares similarities with maintaining consistency between setups across chains, which could also result in problems with token withdrawals.

We have a PoC here:

function test_cancelRequestFail() 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);
​
StarklaneState(bridge).setStarklaneL2Selector(123);
​
vm.expectRevert();
IStarklane(bridge).startRequestCancellation(payload, nonce);
​
}

It could be observed that the hash is different and causes NO_MESSAGE_TO_CANCEL Error.

├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [21871] ERC1967Proxy::startRequestCancellation([257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 0, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], 0)
│ ├─ [21272] Starklane::startRequestCancellation([257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 0, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], 0) [delegatecall]
│ │ ├─ [14900] StarknetMessagingLocal::startL1ToL2MessageCancellation(1, 123, [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 0, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], 0)
│ │ │ ├─ emit MessageToL2CancellationStarted(fromAddress: ERC1967Proxy: [0x0F8458E544c9D4C7C25A881240727209caae20B8], toAddress: 1, selector: 123, payload: [257, 319552616103626275293414929774876113634 [3.195e38], 39080778567855397924224240164501149526 [3.908e37], 243313642115106858902493542147085865830094663267 [2.433e47], 0, 884601108998247062727812365178754106561321340343 [8.846e47], 1, 0, 121364726226993 [1.213e14], 6, 0, 21297 [2.129e4], 2, 0, 0, 0, 2, 0, 0, 9, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0], nonce: 0)
│ │ │ └─ ← [Revert] revert: NO_MESSAGE_TO_CANCEL
│ │ └─ ← [Revert] revert: NO_MESSAGE_TO_CANCEL
│ └─ ← [Revert] revert: NO_MESSAGE_TO_CANCEL

Impact

If the _starklaneL2Address and _starklaneL2Selector variables are changed, it could lead to failures in the cancellation process due to the generation of an incorrect hash. This issue may prevent users from canceling their requests, causing their tokens to be locked within the bridge indefinitely. The impact is significant, as it directly affects the protocol’s ability to handle cross-chain requests reliably.

Tools Used

Manual

Recommendations

To mitigate this issue, it is recommended to introduce an additional privileged function that allows for the retrieval of stuck tokens in scenarios where the cancellation process fails due to changes in _starklaneL2Address or _starklaneL2Selector. This function would serve as a safeguard, ensuring that tokens are not permanently locked in the event of a hash mismatch caused by changes to critical state variables.

Updates

Lead Judging Commences

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

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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