Summary

Tokens that get deposited on L2 to be withdrawn on the L1 and don't get withdraw prior to an update to any of the core addresses are effectively lost

Vulnerability Details

First from this section of the walkthrough video, we can see how the whole logic of L2 to L1 deposit works, and how this is a bit different from L1 to L2 deposits where the deposits are automatically handled on L2 via the l1 handler and there is no need of any manual consumption unlike the L2.

To give a full package of the path on L2 -> L1 deposits.

First the user calls bridge.cairo#deposit_tokens()

• Per the StarkNet's docs, after calling send_message_to_l1_syscall() we then need to call consumeMessageFromL2 on the L1, in the ArkProject's scope this is done via the route below:

After depositing on the L2, we then call the L1 Bridge.sol#withdrawTokens(), and this forwards us to ``Messaging.sol` as seen below https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L153-L215

This implementation then finally calls consumeMessageFromL2() on the starknet's core contract as expected to consume the message before completing the withdrawals, see Messaging#_consumeMessageStarknet()

Here comes the issue.

On both sides of the bridge, both the L1 & the L2, the end addresses of any of the bridges could be changed, which could be due to a bug or an update to introduce more futures or whatever, main thing to note here is that, changing any of these addresses is an expected functionality that could be taken by the admin whenever, on the L1 this can be done via State.sol#L42-L64 and on the L2 this can be done via bridge.cairo#L211-L215, if/when this is done, for the l1/l2 address, all pending transactions to be consumed are effectively lost/stuck on the bridge.

This can be directly coined from the official StarknetMessaging.sol#L138 where we see that the hash of the tx to be verified is to be gotten from the msg.sender and the fromAddress on the L2 which in our case would be _starklaneL2Address that could be changed, so since any or both of these addresses have been changed, the hash is effectively wrong, which makes this check to always revert the transaction.

More proof to back this claim can be noted by the quoted statement from the official Starknet's docs here: https://book.cairo-lang.org/ch16-04-L1-L2-messaging.html#sending-messages-from-starknet-to-ethereum, to quote:

Note: The consumeMessageFromL2 function of the StarknetCore contract is expected to be called from a Solidity contract, and not directly on the StarknetCore contract. The reason of that is because the StarknetCore contract is using msg.sender to actually compute the hash of the message. And this msg.sender must correspond to the to_address field that is given to the function send_message_to_l1_syscall that is called on Starknet.

To add to the above this would be coupled with the _starklaneL2Address_ to compute the hash as shown in the official StarknetMessaging.sol#L138

So in our case after the change in any of the addresses, users attempting to withdraw their previous tokens (that have been withdrawn but not consumed) would not be able to do so and as such their tokens are lost/stuck.

Impact

Users loss/stuck tokens.

Tools Used

Manual review

Recommendations

Easiest approach to solving this would be removing the ability to change the address of any of the l2/l1 bridge addresses.

Best approach however is to have the functionality of where this call is being made from L1 to the starknetcore addresses open, like a claim() functionality and then allow users to pass in the addresses of the l1/l2 address they had use for their attempted bridging, and any active bridge can route the withdrawal attempt via this deprecated/inactive bridge.

Additional Note

NB: Do not consider this last section if reading the report.

To help ease judging, I'd like to indicate that we can't consider this "Admin/Owner error", cause 1. There is no functionality to show that some tokens have been withdrawn and not consumed on the L1, 2. Even if they were, if the admins decide that all tokens must be withdrawn/consumed before they can change any of the l1/l2 addresses, this means any NFT holder can DOS the logic by withdrawing but not consuming, 3. Most importantly in the case of a black swan event, even if they were requests pending consumption the Admins would have no options but to update the addresses and as such these tokens get lost.