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

Unchecking address hash Collision when deploying NFT collection on L2

alqaqa

Vulnerability Details

When We Bridge NFTs from Ethereum to Starknet, we are deploying a new NFT collection if needed.

When making the deploying process, we are using Salt (similar to Create2 in solidity). But the problem is that we are not checking if there is hash collision or not.

collection_manager.cairo#L153-L157

fn deploy_erc721_bridgeable( ... ) -> ContractAddress {
...
​
match starknet::deploy_syscall(class_hash, salt, calldata.span(), false) {
❌️ Result::Ok((addr, _)) => addr,
// TODO: do we want an event emitted here?
Result::Err(revert_reason) => panic(revert_reason)
}
}

As we can see we are returning the address after deploying, but if there is a Hash Collision the function will not revert it will just return address(0) as in Solidity/EVM.

Impact

Completing the process of Bridging tokens even if deploying NFT collection on Layer 2 fails.

Tools Used

Manual review

Recommendations

Check for the returned address, and revert if it was an address(0).

diff --git a/apps/blockchain/starknet/src/token/collection_manager.cairo b/apps/blockchain/starknet/src/token/collection_manager.cairo
index 0d605b0..e150502 100644
--- a/apps/blockchain/starknet/src/token/collection_manager.cairo
+++ b/apps/blockchain/starknet/src/token/collection_manager.cairo
@@ -151,7 +151,12 @@ fn deploy_erc721_bridgeable(
// Last argument false -> set the address of the contract using this function
// as the contract's deployed address.
match starknet::deploy_syscall(class_hash, salt, calldata.span(), false) {
- Result::Ok((addr, _)) => addr,
+ Result::Ok((addr, _)) => {
+ if addr == ContractAddress::zero() {
+ panic!("Address Zero");
+ }
+ addr
+ },
// TODO: do we want an event emitted here?
Result::Err(revert_reason) => panic(revert_reason)
}

NOTE: reverting is not the best choice here, as this will result in Losing Bridged NFTs, but also hash collision will result in failure of the deployment process, which will make the address not the NFT collection we need, and will result in incorrect L1<->L2 collection mappings. So implementing this check with a method to recover NFTs on L2 is a good choice.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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