Summary
In the event of an L2 contract hash upgrade that changes the L2 bridge selector, or a migration to other L2 bridge address, it's possible that there are bridge requests using the previous configuration and those requests will result in failed execution on L2. However, there is no way to recover the NFTs in this case because the cancellation request on the L1 bridge contract does not account for such scenarios.
Vulnerability Details
It is possible that the L2 bridge contract could be upgraded and the selector for the #[l1_handler] that handles bridging on the L2 side could be changed :
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L184-L198
#[abi(embed_v0)]
impl BridgeUpgradeImpl of IUpgradeable<ContractState> {
fn upgrade(ref self: ContractState, class_hash: ClassHash) {
ensure_is_admin(@self);
match starknet::replace_class_syscall(class_hash) {
Result::Ok(_) => {
self.emit(ReplacedClassHash {
contract: starknet::get_contract_address(),
class: class_hash,
})
},
Result::Err(revert_reason) => panic(revert_reason),
};
}
}
Or another scenario, when the L2 bridge contract might be migrated to a different L2 address. This configuration change is possible, and the function to handle such a scenario is available on L1 :
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/State.sol#L42-L64
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);
}
When these scenarios occur, the in-flight bridging requests from L1 to L2 using the old configuration will revert on the L2 side because the L2 contract selector no longer exists or the old L2 contract is disabled. However, the bridging request cannot be canceled because the system does not allow users to provide a custom L2 address and L2 selector. the startRequestCancellation and cancelRequest functions will use the currently configured _starklaneL2Address and _starklaneL2Selector.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L223-L256
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);
}
NOTE : This issue does not require an admin mistake, as normal configuration changes through the available setStarklaneL2Address and setStarklaneL2Selector functions will cause this problem.
Impact
This will prevent all L1 to L2 bridging requests using the previous configuration from being canceled, causing the NFTs to become stuck on L1 with no way to recover them.
Tools Used
Manual review
Recommendations
Allow users to provide _starklaneL2Address and _starklaneL2Selector as parameters when triggering startRequestCancellation and cancelRequest.
function startRequestCancellation(
uint256[] memory payload,
uint256 nonce,
+ snaddress __starklaneL2Address,
+ felt252 __starklaneL2Selector
) external onlyOwner {
IStarknetMessaging(_starknetCoreAddress).startL1ToL2MessageCancellation(
- snaddress.unwrap(_starklaneL2Address),
+ snaddress.unwrap(__starklaneL2Address),
- felt252.unwrap(_starklaneL2Selector),
+ 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
+ snaddress __starklaneL2Address,
+ felt252 __starklaneL2Selector
) external {
IStarknetMessaging(_starknetCoreAddress).cancelL1ToL2Message(
- snaddress.unwrap(_starklaneL2Address),
+ snaddress.unwrap(__starklaneL2Address),
- felt252.unwrap(_starklaneL2Selector),
+ felt252.unwrap(__starklaneL2Selector),
payload,
nonce
);
Request memory req = Protocol.requestDeserialize(payload, 0);
_cancelRequest(req);
emit CancelRequestCompleted(req.hash, block.timestamp);
}
