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

The Bridging Process will revert if the Collection is matched on the destination chain and not matched on the source chain

alqaqa

Vulnerability Details

When Bridging Collections L1<->L2, we are checking if that NFT collection has a pair on the destination chain or not. and if it has an address on the destination, then we use it instead of redeploying new one.

CollectionManager.sol#L111-L149

function _verifyRequestAddresses(address collectionL1Req, snaddress collectionL2Req) ... {
address l1Req = collectionL1Req;
uint256 l2Req = snaddress.unwrap(collectionL2Req);
address l1Mapping = _l2ToL1Addresses[collectionL2Req];
uint256 l2Mapping = snaddress.unwrap(_l1ToL2Addresses[l1Req]);
​
// L2 address is present in the request and L1 address is not.
if (l2Req > 0 && l1Req == address(0)) {
if (l1Mapping == address(0)) {
// It's the first token of the collection to be bridged.
return address(0);
} else {
// It's not the first token of the collection to be bridged,
// and the collection tokens were only bridged L2->L1.
return l1Mapping;
}
}
​
// L2 address is present, and L1 address too.
if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {
revert InvalidCollectionL1Address();
} else if (l2Mapping != l2Req) {
revert InvalidCollectionL2Address();
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}
​
revert ErrorVerifyingAddressMapping();
}

This function (_verifyRequestAddresses) is called whenever we withdraw tokens, where if the request came from L2 Bridge has a valid collectionL1 address (l1Req), we are doing checks that the matching of addresses is the same on both chains.

  • l2Req is the NFT collection we withdrew from on L2, and it should be a valid NFT collection address

  • l1Req is the l2_to_l1_addresses on L2 where if the collection has matching on L2 it will use that address when bridging tokens from L2 to L1.

bridge.cairo#L274

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

So if the NFT collection has an L1<->L2 matching on L2 we will do a check that ensures the NFT collection L1 and L2 addresses on L2Bridge are the same in L1Bridge.

if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {
revert InvalidCollectionL1Address();
} else if (l2Mapping != l2Req) {
revert InvalidCollectionL2Address();
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}

The problem is that setting l1<->l2 addresses on the L2Bridge doesn't mean that that value is always set on L1Bridge.

We are only setting l1<->l2 on the chain we are withdrawing from, so when bridging from L1 to L2. L2Bridge will set the matching between collections but L1 will not set that matching. So if we tried to withdraw from L2 to L1 the withdrawing will revert as it will compare a collection address with the address zero.

Scenario

  • There is an NFT collection on L1, this collection has no matching on either L1 or L2

  • UserA bridged tokens from that collections to L2

  • req.collectionL2 is address(0) as the is no l1<->l2 matching on L1Bridge

function depositTokens( ... ) ... {
...
req.collectionL2 = _l1ToL2Addresses[collectionL1];
...
}

  • Starknet Sequencer called withdraw_auto_from_l1

  • calling ensure_erc721_deployment to check if the collection on L1 has an address on L2 or not

fn withdraw_auto_from_l1(...) {
...
​
@> let collection_l2 = ensure_erc721_deployment(ref self, @req);

  • Verify Collection address

fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
​
let l1_req: EthAddress = *req.collection_l1;
let l2_req: ContractAddress = *req.collection_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),
);

  • l1_req is the original NFT collection address on L1, l2_req is address(0) as we show when calling L1Bridge::depositTokens() and the other two parameters are also address(0) as there is no matching in L2

  • verify_collection_address() will return address_zero as l2_req is address(0) and there is no matching.

n verify_collection_address(
l1_req: EthAddress,
l2_req: ContractAddress,
l1_bridge: EthAddress,
l2_bridge: ContractAddress,
) -> ContractAddress {
...
if l2_req.is_zero() {
if l2_bridge.is_zero() {
// It's the first token of the collection to be bridged.
❌️ return ContractAddressZeroable::zero();
}
} else { ... }
}

  • Since the returned value is address zero, we will deploy a new address for that L1 Collection on L2, and match it on L2

let l2_addr_from_deploy = deploy_erc721_bridgeable( ... );
​
self.l1_to_l2_addresses.write(l1_req, l2_addr_from_deploy);
self.l2_to_l1_addresses.write(l2_addr_from_deploy, l1_req);

  • Now L2 has collection matched but L1 has not.

  • UserA tried to Bridge Tokens from the same collection but this time from L2 to L1.

  • Calling L2Bridge::deposit_tokens(), and req.collection_l1 will be the original L1 collection address that is matched to the L2 collection address we deployed in the prev steps.

fn deposit_tokens( ... ) {
...
let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
...
}

  • Starknet Sequencer accepted the message after verification.

  • UserA called L1Bridge::withdrawTokens(), and we will verify the request.

function withdrawTokens( ... ) ... {
...
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
...
}

  • req.collectionL1 is the original NFT address on L1 we got it as there is a matching on L2 and req.collectionL2 is the address we deployed on L2 for that collection when we first bridge from L1 to L2, so both values are set with values.

  • Inside _verifyRequestAddresses, l1Req and l2Req have correct values as illustrated in the prev step, so we will go on to the second if condition.

function _verifyRequestAddresses(
address collectionL1Req,
snaddress collectionL2Req
)
internal
view
returns (address)
{
address l1Req = collectionL1Req;
uint256 l2Req = snaddress.unwrap(collectionL2Req);
address l1Mapping = _l2ToL1Addresses[collectionL2Req];
uint256 l2Mapping = snaddress.unwrap(_l1ToL2Addresses[l1Req]);
​
// L2 address is present in the request and L1 address is not.
if (l2Req > 0 && l1Req == address(0)) { ... }
​
// L2 address is present, and L1 address too.
@> if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {
❌️ revert InvalidCollectionL1Address();
} else if (l2Mapping != l2Req) {
revert InvalidCollectionL2Address();
} else {
// All addresses match, we don't need to deploy anything.
return l1Mapping;
}
}
​
revert ErrorVerifyingAddressMapping();
}

  • Since there is no matching in L1, l1Mapping and l2Mapping are address(0), and will go for the check l1Mapping != l1Req, which will be true, ending up withdrawing from L1 getting reverted.

Proof of Concept

Add the following test function function in apps/blockchain/ethereum/test/Bridge.t.sol.

function test_auditor_collection_matching_one_chain() public {
// alice deposit token 0 and 9 of collection erc721C1 to bridge
test_depositTokenERC721();
​
// Build the request and compute it's "would be" message hash.
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
​
// Build Request on L2
Request memory req = buildRequestDeploy(header, 9, bob);
req.collectionL1 = address(erc721C1);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
​
// The message must be simulated to come from starknet verifier contract
// on L1 and pushed to starknet core.
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
​
// Withdrawing tokens will revert as There is no matching on L1
address collection = IStarklane(bridge).withdrawTokens(reqSerialized);
}

In the cmd write the following command.

forge test --mt test_auditor_collection_matching_one_chain -vv

Output

The function will revert with an error message InvalidCollectionL1Address()

Ran 1 test for test/Bridge.t.sol:BridgeTest
[FAIL. Reason: InvalidCollectionL1Address()] test_auditor_collection_matching_one_chain() (gas: 648188)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.71ms (1.77ms CPU time)

Impacts

Lossing of NFTs Bridged from L2 to L1

Tools Used

Manual Review + Foundry

Recommendations

Do not check the correct l1<->l2 matching on L1 and L2 if the L1 has no matching yet.

diff --git a/apps/blockchain/ethereum/src/token/CollectionManager.sol b/apps/blockchain/ethereum/src/token/CollectionManager.sol
index ec9429a..f790f70 100644
--- a/apps/blockchain/ethereum/src/token/CollectionManager.sol
+++ b/apps/blockchain/ethereum/src/token/CollectionManager.sol
@@ -113,7 +113,6 @@ contract CollectionManager {
snaddress collectionL2Req
)
internal
- view
returns (address)
{
address l1Req = collectionL1Req;
@@ -133,6 +132,13 @@ contract CollectionManager {
}
}
+ // L2 is present, L1 address too, and there is no mapping
+ if (l2Req > 0 && l1Req > address(0) && l1Mapping == address(0) && l2Mapping == 0) {
+ _l1ToL2Addresses[l1Req] = collectionL2Req;
+ _l2ToL1Addresses[collectionL2Req] = l1Req;
+ return l1Req;
+ }
+
// L2 address is present, and L1 address too.
if (l2Req > 0 && l1Req > address(0)) {
if (l1Mapping != l1Req) {

Existence of the issue on the Starknet Side

We illustrated the issue when Bridging tokens from L2 to L1 and they have matchings on L2 but not in L1, this issue existed also when Bridging from L1 to L2 when L1 has a matching but L2 has not. where the implementaion of the verification function is the same.

collection_manager.cairo#L170-L200

fn verify_collection_address( ... ) -> ContractAddress {
​
// L1 address must always be set as we receive the request from L1.
if l1_req.is_zero() {
panic!("L1 address cannot be 0");
}
​
// L1 address is present in the request and L2 address is not.
if l2_req.is_zero() { ... } else {
// L1 address is present, and L2 address too.
if l2_bridge != l2_req {
❌️ panic!("Invalid collection L2 address");
}
​
if l1_bridge != l1_req {
panic!("Invalid collection L1 address");
}
}
​
l2_bridge
}

If l1_req and l2_req have values (there is a matching in L1) we will go for the else block, and since l2_bridge is address_zero (no matching on L2) withdrawing process will revert on L2.

The scenario is the same as we illustrated but with replacing L1 by L2 and L2 by L1. So to not repeat ourselves we mentioned it here briefly.

To mitigate the issue on L2 we will do the same as in L1 where if there is no matching in L2 but there is in L1 we will just return the addresses.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year 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.

Appeal created

nirohgo Auditor
about 1 year ago
n0kto Lead Judge
about 1 year ago
n0kto Lead Judge about 1 year 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.