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

Users that bridged a token from native L2 collection to L1 can't bridge it back

0xasen

Summary

If you bridge native L2 collection to L1, you can't bridge it back because of a failed if statement in verify_collection_address. This check will fail:

if l2_bridge != l2_req {
panic!("Invalid collection L2 address");
}

Vulnerability Details

Let's consider the scenario of having a native L2 collection, not yet bridged. Now let's follow the flow of bridging a token and then bridging it back.
We call deposit_tokens() on the L2 contract, collection_l2 is the address of the collection on the L2 and this returns address(0) since the l1 -> l2 mappings are not updated because we bridge for the first time:

let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);

Now, the message is processed and we call withdrawTokens() on the L1 contract. This returns address(0) since the collection is yet to be deployed on the L1:

address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);

and we enter this if:

if (collectionL1 == address(0x0)) {
//and the type is erc721
if (ctype == CollectionType.ERC721) {
//deploy the collection
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);

which proceeds to deploy the collection and correctly updates the _l1ToL2Addresses and _l2ToL1Addresses mappings:

function _deployERC721Bridgeable(
string memory name,
string memory symbol,
snaddress collectionL2,
uint256 reqHash
)
internal
returns (address)
{
address proxy = Deployer.deployERC721Bridgeable(name, symbol);
​
_l1ToL2Addresses[proxy] = collectionL2;
​
_l2ToL1Addresses[collectionL2] = proxy;
​
return proxy;
}

Now let's try and bridge back the same token. We call depositTokens() on L1 and these expressions return the stored addresses for the collection both for L1 and L2 since we updated them with the deployment of the collection on L1:

req.collectionL1 = collectionL1;
req.collectionL2 = _l1ToL2Addresses[collectionL1];

The request is processed and withdraw_auto_from_l1() is called on the L2. Let's focus on this line:

let collection_l2 = ensure_erc721_deployment(ref self, @req);

and focus on the first lines of the function:

fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
​
//extract the L1 and L2 collection addresses from the request
let l1_req: EthAddress = *req.collection_l1;
let l2_req: ContractAddress = *req.collection_l2;
​
//verify if the collection address is valid and deployed on L2
let collection_l2 = verify_collection_address(
l1_req,
l2_req,
self.l2_to_l1_addresses.read(l2_req),
self.l1_to_l2_addresses.read(l1_req),
);

Here the values for l1_req and l2_req will be the corresponding addresses of the collection on L1 and L2 since that's what we passed to the request. However, self.l2_to_l1_addresses.read(l2_req) and self.l1_to_l2_addresses.read(l1_req) both will be address(0) since these mappings were never updated.

Now let's see the verify_collection_address() function:

fn verify_collection_address(
l1_req: EthAddress,
l2_req: ContractAddress,
l1_bridge: EthAddress,
l2_bridge: ContractAddress,
) -> ContractAddress {
​
if l1_req.is_zero() {
panic!("L1 address cannot be 0");
}
​
​
if l2_req.is_zero() {
if l2_bridge.is_zero() {
return ContractAddressZeroable::zero();
}
} else {
if l2_bridge != l2_req {
panic!("Invalid collection L2 address");
}
if l1_bridge != l1_req {
panic!("Invalid collection L1 address");
}
}
​
l2_bridge
}

Since l2_req is not zero we'll enter the else statement and this will fail:

if l2_bridge != l2_req {
panic!("Invalid collection L2 address");
}

because l2_bridge represents the address of the L2 collection stored in the bridge contract which currently is 0, while the l2_req represents the address we passed with the request, which is not 0.

Impact

When users bridge a native L2 collection from L2 -> L1 they won't be able to bridge back breaking the most critical functionality of the protocol

Tools Used

Manual review

Recommendations

I think it's safe to assume the l2_req value is correct since it's the L1 bridge contract that passes it. So you can just return l2_req in the else statement.

Updates

Lead Judging Commences

n0kto Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-first-bridgeof-a-collection-L1<->L2-do-not-sync-addresses

Likelyhood: High, any collections bridged, without bridge owner action, will be unable to bridge back. Impact: High, L2 -> L1 tokens will be stuck in the bridge. L1 -> L2 will need to ask for a cancellation.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.