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

Missed event for admin operations (2 findings) and lack of parameters checks (3 findings).

calfun0x

Hi 👋, this is my first report for Cyfrin so i hope to comply with all findings requirements..😬

Summary (1/5)

Missed event for admin operation in cairo functions

Vulnerability Details

The functions enable_white_list, set_bridge_l1_addr, set_erc721_class_hash and set_l1_l2_collection_mapping in bridge.cairo lacks of events for
administrative writing operations which is a best practice for admin operations. (Same functions in solidity already emit events)

fn enable_white_list(ref self: ContractState, enable: bool) {
ensure_is_admin(@self);
self.white_list_enabled.write(enable);
}
​
fn set_bridge_l1_addr(ref self: ContractState, address: EthAddress) {
ensure_is_admin(@self);
self.bridge_l1_address.write(address);
}
​
fn set_erc721_class_hash(ref self: ContractState, class_hash: ClassHash) {
ensure_is_admin(@self);
self.erc721_bridgeable_class.write(class_hash);
}
​
fn set_l1_l2_collection_mapping(ref self: ContractState, collection_l1: EthAddress, collection_l2: ContractAddress) {
ensure_is_admin(@self);
self.l1_to_l2_addresses.write(collection_l1, collection_l2);
self.l2_to_l1_addresses.write(collection_l2, collection_l1);
}

Impact

Lost of critical records to audit administrator activities and keep transparency for third parties.

Tools Used

VSCode

Recommended Mitigation

Emit an event after change adminitrative variables status.

Summary (2/5)

Missed event for admin operation in solidity functions

Vulnerability Details

The function enableBridge in bridge.sol lacks of events for administrative writing operations which is a best practice for admin operations. (Same function in cairo already have events)

function enableBridge(
bool enable
) external onlyOwner {
_enabled = enable;
}

Impact

Lost of critical records to audit administrator activities and keep transparency for third parties.

Tools Used

VSCode

Recommended Mitigation

Emit an event after change adminitrative variables status.

Summary (3/5)

Unchecked parameter L1 bridge address

Vulnerability Details

The function set_bridge_l1_addr in bridge.cairo lack checks for address parameter exposing the platform to deny deposits for users in Starknet in case of
an admin set this value to zero (intentionally or unintentionally).

1)Missing assert for zero bridge address:

fn set_bridge_l1_addr(ref self: ContractState, address: EthAddress) {
ensure_is_admin(@self);
self.bridge_l1_address.write(address);
}

2)Then deposit_tokens will reject deposits for a zero address bridge:

fn deposit_tokens(
ref self: ContractState,
salt: felt252,
collection_l2: ContractAddress,
owner_l1: EthAddress,
token_ids: Span<u256>,
use_withdraw_auto: bool,
use_deposit_burn_auto: bool,
) {
ensure_is_enabled(@self);
assert(!self.bridge_l1_address.read().is_zero(), 'Bridge is not open');

Impact

Deposits denegation for users in case of an admin set this value to zero (intentionally or unintentionally).

Tools Used

VSCode

Recommended Mitigation

Add a nonzero check in set_bridge_l1_addr function for address parameter.

Summary (4/5)

Unchecked parameter class hash

Vulnerability Details

The function set_erc721_class_hash in bridge.cairo lack checks for zero class_hash parameter exposing the platform to deny request from L1 with
new L2 collections if is intentionally or unintentionally asigned to zero.

1)Missing assert for zero class hash:

fn set_erc721_class_hash(ref self: ContractState, class_hash: ClassHash) {
ensure_is_admin(@self);
self.erc721_bridgeable_class.write(class_hash);
}

2)Then, a request from L2 calls this L1 handler function in Starknet and handler calls ensure_erc721_deployment function:

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
// ...
let collection_l2 = ensure_erc721_deployment(ref self, @req);

3)Then ensure_erc721_deployment function will try to deploy a zero class hash ERC721 in L2 leading to reject the bridged NFT collection from L1:

fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
// ...
​
if !collection_l2.is_zero() {
return collection_l2;
}
​
// ...
​
let l2_addr_from_deploy = deploy_erc721_bridgeable(
self.erc721_bridgeable_class.read(),
salt,
req.name.clone(),
req.symbol.clone(),
req.base_uri.clone(),
starknet::get_contract_address(),
);

Impact

The platform will fail to process L1 to L2 messages for new L2 collections in case of an admin set this value to zero (intentionally or unintentionally).

Tools Used

VSCode

Recommended Mitigation

Add a nonzero check in set_erc721_class_hash function.

Summary (5/5)

Unchecked parameters collections

Vulnerability Details

The function set_l1_l2_collection_mapping in bridge.cairo lack checks for zero collection_l1 and collection_l2 parameters exposing the platform to deny
requests from L1.

1)Missing asserts for zero addresses collections:

fn set_l1_l2_collection_mapping(ref self: ContractState, collection_l1: EthAddress, collection_l2: ContractAddress) {
// ...
self.l1_to_l2_addresses.write(collection_l1, collection_l2);
self.l2_to_l1_addresses.write(collection_l2, collection_l1);
}

2)Then, a request from L2 calls this L1 handler function in Starknet and handler calls ensure_erc721_deployment function:

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
// ...
let collection_l2 = ensure_erc721_deployment(ref self, @req);

3)Then ensure_erc721_deployment function will try to check L1 and L2 address consistency and this check will fail:

fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
// ...
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),
);

Impact

The platform will deny bridged collections from L1 in case of an admin set this value to zero (intentionally or unintentionally).

Tools Used

VSCode

Recommended Mitigation

Add a nonzero check in set_l1_l2_collection_mapping function

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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