Summary

The argument owner_l1 which is the recipient of the deposit action on L2 side is not checked for non-zero value in deposit_tokens function in bridge.cairo. When the withdrawTokens function is called on L1 side - it will revert due to owner_l1 being address(0). This will cause the NFT to remain stuck in escrow on L2 side.

Vulnerability Details

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L242 - Amongst the arguments for this function is owner_l1 which is an EthAddress type. This can be 0 and it is not checked inside the function for non-zero value. The message will go through to L1 side since the request is valid here.
This was verified using this function https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/tests/bridge_t.cairo#L566 in the tests for starknet.
where the following change was made let OWNER_L1 = EthAddress { address: 0_felt252 };

snforge test deposit_token_whitelisted - passed

The L1 side uses OZ version of ERC721 which will revert when "to" address is 0 for either minting or transferFrom calls.
Ref https://github.com/OpenZeppelin/openzeppelin-contracts/blob/eb4e8632f781cdc86b4b47c7e80a5066499bd5d8/contracts/token/ERC721/ERC721.sol#L138
and
Ref https://github.com/OpenZeppelin/openzeppelin-contracts/blob/eb4e8632f781cdc86b4b47c7e80a5066499bd5d8/contracts/token/ERC721/ERC721.sol#L286

The following change was made to the test functions at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/test/Bridge.t.sol#L298
and at https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/test/Bridge.t.sol#L403

Request memory req = buildRequestDeploy(header, 9, address(0)); the 3rd argument here is the proposed owner_l1 address for the transfer from L2 side.

Following tests failed

forge test --match-test test_depositWithdrawTokens_withMapping

forge test --match-test test_withdrawTokensERC721StarknetWithdrawDeploy

This means that the NFT will get stuck in escrow on L2 side since tx was valid on one-side of the bridge only.

Impact

The probability of such a situation happening is low but the damage is severe since the NFT will permanently get stuck in escrow on the L2 side. However, even though the responsibility of using correct owner_l1 address lies with the depositor from Starknet side, the fix for this is a simple check in deposit_tokens function and will prevent this situation from happening.

Tools Used

Manual review and snforge test and forge test

Recommendations

Introduce a simple check
assert(!owner_l1.is_zero(), 'Owner L1 cannot be 0');
inside deposit_tokens function in bridge.cairo