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

It won't be possible to cancel an old L1 to L2 deposit request if `_starklaneL2Address` or `_starklaneL2Selector` is changed

sammy

Summary

It won't be possible to cancel an old L1 to L2 deposit request if _starklaneL2Address or _starklaneL2Selector is changed. This will lead to NFTs being stuck in the contract.

Vulnerability Details

Currently, the codebase supports the cancellation of L1 to L2 NFT deposit requests with the following sequence of actions :

step 1 : Bridge owner calls startRequestCancellation() in Bridge.sol for the deposit request.
step 2 : A user calls cancelRequest() in Bridge.sol to finalize the cancellation request and withdraw the NFTs from the escrow.

This seems pretty straightforward, however, if either of _starklaneL2Address or _starklaneL2Selector is altered before or after step 1, step 2 will revert. This is because in both the startRequestCancellation() and cancelRequest() functions, the storage variables _starklaneL2Address and _starklaneL2Selector are used to calculate the msgHash in StarknetMessaging.sol.

Bridge.sol :

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);
}
​
/**
@notice Cancel a given request.
​
@param payload Request to cancel
@param nonce Nonce used for request sending.
*/
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);
}

StarknetMessaging.sol :

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;
}
​
function cancelL1ToL2Message(
uint256 toAddress,
uint256 selector,
uint256[] calldata payload,
uint256 nonce
) external override returns (bytes32) {
emit MessageToL2Canceled(msg.sender, toAddress, selector, payload, nonce);
// Note that the message hash depends on msg.sender, which prevents one contract from
// cancelling another contract's message.
// Trying to do so will result in NO_MESSAGE_TO_CANCEL.
bytes32 msgHash = getL1ToL2MsgHash(toAddress, selector, payload, nonce);
uint256 msgFeePlusOne = l1ToL2Messages()[msgHash];
require(msgFeePlusOne != 0, "NO_MESSAGE_TO_CANCEL");
​
uint256 requestTime = l1ToL2MessageCancellations()[msgHash];
require(requestTime != 0, "MESSAGE_CANCELLATION_NOT_REQUESTED");
​
uint256 cancelAllowedTime = requestTime + messageCancellationDelay();
require(cancelAllowedTime >= requestTime, "CANCEL_ALLOWED_TIME_OVERFLOW");
require(block.timestamp >= cancelAllowedTime, "MESSAGE_CANCELLATION_NOT_ALLOWED_YET");
​
l1ToL2Messages()[msgHash] = 0;
return (msgHash);
}

There are two possible scenarios :

  1. The owner alters _starklaneL2Address or _starklaneL2Selector before step 1
    The owner won't be able to initiate the cancellation of a deposit request that uses the old values of the variables.

  2. The owner alters _starklaneL2Address or _starklaneL2Selector after step 1
    The user won't be able to finalize the cancellation of a deposit request that uses the old values of the variables.

In theory, the owner can always switch the variables back to their old values and allow the cancellation to occur, however, during this window, new deposit requests created would be ambiguous and would need further mitigation, resulting in a neverending cycle.

Impact

Stuck NFTs/ DOS

Tools Used

Recommendations

Add _starklaneL2Address , _starklaneL2Selector as input variables in both the startRequestCancellation() and cancelRequest() functions.

function startRequestCancellation(
uint256[] memory payload,
- uint256 nonce
+ uint256 nonce,
+ snaddress starklaneL2Address,
+ felt252 starklaneL2Selector
+
) external onlyOwner {
IStarknetMessaging(_starknetCoreAddress).startL1ToL2MessageCancellation(
- snaddress.unwrap(_starklaneL2Address),
- felt252.unwrap(_starklaneL2Selector),
+ snaddress.unwrap(starklaneL2Address),
+ felt252.unwrap(starklaneL2Selector),
payload,
nonce
);
​
​
function cancelRequest(
uint256[] memory payload,
- uint256 nonce
+ uint256 nonce,
+ snaddress starklaneL2Address,
+ felt252 starklaneL2Selector
) external {
IStarknetMessaging(_starknetCoreAddress).cancelL1ToL2Message(
- snaddress.unwrap(_starklaneL2Address),
- felt252.unwrap(_starklaneL2Selector),
+ snaddress.unwrap(starklaneL2Address),
+ felt252.unwrap(starklaneL2Selector),
payload,
nonce
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice
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.