[L-01] initialize will fail if the owner is not the sender

Summary

If a new bridge is deployed, it is initialized with a call to initialize. This sets the owner, the starknet core address and some more. Currently the owner gets specified via the variable owner which is passed to the function.
The problem is, that this call will revert if the specified owner is not the msg.sender because the owner is set before initialization is finished.

Vulnerability Details

If we look at initialize, we see that after _transferOwnership(owner) is called, setStarklaneL2Address(starklaneL2Address) and setStarklaneL2Selector(starklaneL2Selector) are called.

The problem arises, because both functions called after _transferOwnership, have the onlyOwner modifier which checks whether the caller is the owner.

This means that we have to call _transferOwnership, but it also means if we set the owner to something other than msg.sender, this will revert.

Impact

This makes it impossible to initialize a bridge with an owner other than the caller which could definitely be the case.

Proof of Concept

In order to show this, please do the following:

• Add the following line to the setUp function in Bridge.t.sol

  • add vm.prank(address(0xdeadbeef)) before the call to bridge = address(new ERC1967Proxy(impl, dataInit)) to simulate someone initializing the bridge for someone else

• Execute any test that uses the bridge, for example test_depositTokenERC721, by calling forge test --match-test test_depositTokenERC721

• This will revert with the error Ownable: caller is not the owner showing that we cannot initialize for someone else

Tools Used

Manual review

Recommended Mitigation

In order to prevent this and be more flexible with initialization I would suggest not calling these functions but setting the variables directly as it is done with _starknetCoreAddress.

[L-02] Collections implementing both ERC721 and ERC1155 are always interpreted as ERC721

Summary

If a user wants to bridge an NFT of a collection that supports both ERC721 and ERC1155, the token will always be interpreted as an ERC721 token.

Vulnerability Details

Currently the distinction between ERC721 and ERC1155 tokens is done in detectInterface in TokenUtil.sol.

Looking at it, we can see that the function returns the CollectionType ERC721 if the collection supports that interface.
Now a collection can technically support both ERC721 and ERC1155 interfaces. If that is now the case, this collection's ERC1155 NFTs cannot be bridged as they would be interpreted as ERC721 tokens.

Impact

This issue prevents the bridge from being properly usable for hybrid NFT collections implementing multiple standards in one contract.
Now it is not very likely for such a collection to exist but since it is technically possible, this should be considered.

Tools Used

Manual review

Recommended Mitigation

I would recommend changing the CollectionType to be user-provided. The bridge would then need to verify that the type is either ERC721 or ERC1155 and then check if that collection actually implements that interface.
This would allow users of hybrid collections to bridge both ERC721 and ERC1155 tokens of that collection.

[L-03] No auto-burn functionality even though described in the specs

Summary

Currently the auto-burn functionality (useAutoBurn on L1, use_deposit_burn_auto on L2) can be enabled by the user but the flag is not used on either side when withdrawing NFTs.

Vulnerability Details

If we look at the IStarklane.sol code, we can see the following comment: @param useAutoBurn If true, ensures the token is burnt after being bridged.. This implies, that the useAutoBurn flag is used and is expected to burn tokens after bridging them. The problem is, that this flag can be set (on both L1 and L2) but there is no functionality that gets triggered when enabling it.

Impact

This means that users relying on the documentation of IStarklane.sol will encounter unexpected behaviour, specifically that their NFTs will not be burnt but instead escrowed in the bridge.

Tools Used

Manual Review

Recommended Mitigation

I would recommend either implementing the described functionality or clearly specifying that setting this flag currently has no effect on the executed logic.

[L-04] setL1L2CollectionMapping never checks whether collectionL2 is in fact a snaddress

Summary

Bridge::setL1L2CollectionMapping takes a snaddress as an argument to set the collection mappings on L1. Since the snaddress is a uint256 and never checked to be in-bounds to be a valid snaddress, this can lead to accidentally setting the mapping incorrectly.

Vulnerability Details

Such checks are done in for example depositTokens:

Looking at setL1L2CollectionMapping, unfortunately there are no such checks in place:

Impact

If the mapping accidentally gets set incorrectly due to this, this would cause major issues with bridging for that collection such as stuck NFTs and in the worst case loss of NFTs.

Tools Used

Manual review

Recommended Mitigation

To be sure the collection2 address is a snaddress, I would recommend introducing a similar check to the one in depositTokens.

[L-05] Collections can change their implementation arbitrarily after being whitelisted and stay whitelisted

Summary

Since the bridge's whitelist contains proxies, collections can change their implementation arbitrarily, possibly not confirming to ArkProject's standards anymore.

Vulnerability Details

The whitelist should only contain collections of which the admins approve and which implement all needed functionality to be bridged. Now if a collection utilizes a proxy, that proxy address is added to the whitelist.
If now the collection owners change the implementation, this is not detected by the bridge, leading to a non-vetted collection to be part of the whitelist which should be prevented.

Impact

As described above this causes the whitelist to contain unverified collections which should not be the case as the whitelist should only consist of collections directly approved by the project.

Tools Used

Manual review

Recommended Mitigation

It would possibly make sense to add another check to _isWhiteListed (and also on L2) for whether the implementation is still the same as when it was whitelisted. This would need to be recorded for proxies when they are added to the whitelist.

[L-06] No checks for whether l1_to_l2_addresses and l2_to_l1_addresses mappings are already set on L2

Summary

On L2 there is no force flag needed to overwrite an already set address-mapping. This can in the worst case lead to loss of NFTs.

Vulnerability Details

If we look at L1, there is a check for whether out address mapping is already set and if it is, we can only overwrite it if we specify a force flag. This makes sense because accidentally overwriting this mapping can cause users to lose NFTs in the worst case.

The problem is that this is not the case on L2 and the mapping can just be overwritten making it easier for mistakes to happen.

Impact

If now an admin accidentaly overwrites an existing mapping this will lead to incorrect bridging, having NFTs stuck in the bridge or in the worst case calls made to wrong addresses, leading to loss of NFTs.

Tools Used

Manual review

Recommended Mitigation

I would recommend adding a check on L2 similar to the one on L1, needing a force flag to confirm we want to overwrite an existing entry.

[L-07] No checks whether requesthash has already been used

Summary

According to the README: An any attempt to replay a request with the same salt is denied..
Therefore there should be some checks in for example Bridge.sol for whether a requesthash or a salt has already been used which is not the case.

Impact

In addition to not following the specs, this has a more severe impact if auto withdrawals are enabled. This is because with _consumeMessageAutoWithdraw, a request with a message hash H1 can only be consumed once as its state will be set before execution finishes.
If now a user tries to bridge again with the same request (essentially bridging to the same user on L1 again after it has been bridged back), the call to _consumeMessageAutoWithdraw would fail due to the following check:

This would mean the NFT is locked in the L1 bridge.

Proof of Concept

This can be demonstrated by spinning up a local testnet according to the sponsor's description and doing the following:

1. Send NFT from L1 to L2 with the salt 10

2. Send the NFT back from L2 to L1

3. Try to send the NFT again to L2 using the exact same command as in 1)

Step 3) will succeed, showing that there are in fact no restrictions on the salt or request hash.

Recommended Mitigation

In order to conform to the documentation and prevent issues with auto withdrawals in the future, there should be checks implemented for whether a requesthash has already been used on both L1 and L2.

[L-08] L1 bridge does not enforce a minimum value sent to depositTokens making it possible for NFTs to be stuck

Summary

Currently a user can pass any value to depositTokens. This value is used to pay for inclusion/transaction fees on L2. If this value is too low, the transaction will not be included on L2.

Vulnerability Details

The issue is that users can pass almost arbitrary msg.value to the depositTokens function as the function enforces no minimums and does not do any calculations on expected transaction cost on L2.

Impact

This means that NFTs may be locked in the bridge on L1 if the user does not provide a big enough msg.value for it to be included. This can happen accidentally as there are no restrictions on the value.

Tools Used

Manual review

Recommended Mitigation

Consider adding a calculation of the expected transaction cost on L2 based on the payload size.

[L-09] Anyone can cancel a message once cancellation was requested

Summary

Currently the function startRequestCancellation can only be called by the owner, only allowing him to request to cancel a message to L2. The function cancelRequest however can be called by anyone. Now this means that if I request cancellation for my request and the admin requests it for me, I can then cancel it once the waiting period is over.

The problem is that there are no checks whether the caller of the function is the owner of the NFT on L1. This means anyone can cancel a message for which I requested cancellation. The NFT will still be transferred to me as it takes req.ownerL1 as the receiver but as the owner I should still be the one making the decision whether I want to cancel the message.

Recommended Mitigation

Consider adding a check to cancelRequest for whether the request owner on L1 equals the msg.sender.

[L-10] No check in mint_range for whether start > end

Summary

The mint_range function in erc721_bridgeable.cairo allows an admin to mint a range of NFTs ranging from start to end. The issue is that there are no checks enforcing whether start > end causing an inifinite loop, always reverting the transaction with an unknown error.

It is recommended to add a check for whether start > end as it would prevent such loops from happening early and provide the admin with a comprehensive error message.

[L-11] No checks whether collection supports ERC721 interface on L2

Summary

Currently the L2 side does not check whether a collection actually implements the ERC721 interface.

Vulnerability Details

On L1, there is currently a check whether the bridged collection implements the ERC721 interface as specified in ERC165. The problem is that this check is not added on L2 resulting in users being able to call deposit_tokens with a collection which does not implement all the needed functions.
This will cause unexpected behaviour.

Therefore it is recommended to add a check for whether the passed collection implements the interface and only allow deposit_tokens to be called with collections that do so.