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

`L2Bridge` Deployes Collections before checking there type

alqaqa

Vulnerability Details

When Bridging tokens L1->L2, we are deploying a new address for that Bridged token NFT collection on L1 on L2 if it has no attacked address on L2.

The problem is that in L2Bridge::withdraw_auto_from_l1() we are always deploying an ERC721 address without checking the ctype first.

bridge.cairo#L141-L143

#[l1_handler]
fn withdraw_auto_from_l1( ... ) {
...
​
1: let collection_l2 = ensure_erc721_deployment(ref self, @req);
​
2: let _ctype = collection_type_from_header(req.header);
}

As we can see we deploy the collection address in the first as an ERC721, without checking this type either ERC721 or ERC1155.

Although this should not result in any problems unless data altered when Bridged in the current implementation, this is no the way the L1Bridge work when withdrawing. where we are checking for the ctype before we deploy new collections.

Bridge.sol#L181-L196

1: CollectionType ctype = Protocol.collectionTypeFromHeader(header);
​
if (collectionL1 == address(0x0)) {
2: if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable( ... );
// update whitelist if needed
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}

As we can see in L1 Bridge, we are checking for the type, and if it is ERC721 we are deploying an ERC721 collection, and if not we are reverting the tx. which is not the case in L2 Bridge withdrawing which does not preventing Bridging ERC1155 tokens to be Bridged.

Tools Used

Manual Review

Recommendations

Check that the ctype is ERC721 before deploying, and if not revert the tx

diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo
index 23cbf8a..95e909b 100644
--- a/apps/blockchain/starknet/src/bridge.cairo
+++ b/apps/blockchain/starknet/src/bridge.cairo
@@ -138,9 +138,11 @@ mod bridge {
// TODO: recompute HASH to ensure data are not altered.
// TODO: Validate all fields the request (cf. FSM).
+ let _ctype = collection_type_from_header(req.header);
+ assert(_ctype == CollectionType::ERC721, 'Not Supported Collection Type')
+
let collection_l2 = ensure_erc721_deployment(ref self, @req);
- let _ctype = collection_type_from_header(req.header);
// TODO: check CollectionType to support ERC1155 + metadata.
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

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

Give us feedback!