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

withdraw_auto_from_l1 function in bridge.cairo does not check whether previously deployed collection on L2 is still whitelisted

udayj

Summary

withdraw_auto_from_l1 function in bridge.cairo does not check whether previously deployed collection on L2 is still whitelisted.
This means a transaction can go through to a collection which has been removed from the whitelist.

Vulnerability Details

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L141
withdraw_auto_from_l1 gets the address of collection_l2 from the function ensure_erc721_deployment


https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L428
- The function ensure_erc721_deployment deploys a new collection, if it is not already deployed, and whitelists it. But for an existing collection the address is simply returned ref https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L440

The main withdraw_auto_from_l1 function does not then check whether this address is still whitelisted.

The following test shows how an NFT could be transferred from L1 to a collection which was not whitelisted.

#[test]
fn withdraw_token_with_mapping() {
// Need to declare here to get the class hash before deploy anything.
let erc721b_contract_class = declare("erc721_bridgeable");
​
let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>();
let BRIDGE_L1 = EthAddress { address: 'starklane_l1' };
let COLLECTION_OWNER = starknet::contract_address_const::<'collection owner'>();
let OWNER_L1 = EthAddress { address: 'owner_l1' };
​
let bridge_address = deploy_starklane(BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash);
​
let erc721b_address = deploy_erc721b(
erc721b_contract_class,
"everai",
"DUO",
bridge_address,
COLLECTION_OWNER
);
​
let erc721 = IERC721Dispatcher { contract_address: erc721b_address };
mint_range(erc721b_address, COLLECTION_OWNER, COLLECTION_OWNER, 0, 10);
​
let bridge = IStarklaneDispatcher { contract_address: bridge_address };
​
start_prank(CheatTarget::One(erc721b_address), COLLECTION_OWNER);
erc721.set_approval_for_all(bridge_address, true);
stop_prank(CheatTarget::One(erc721b_address));
let mut spy = spy_events(SpyOn::One(bridge_address));
start_prank(CheatTarget::One(bridge_address), COLLECTION_OWNER);
bridge.deposit_tokens(
0x123,
erc721b_address,
OWNER_L1,
array![0, 1].span(),
false,
false);
stop_prank(CheatTarget::One(bridge_address));
​
let owner_l2 = starknet::contract_address_const::<'owner_l2'>();
let collection_l1 = EthAddress { address: 0x4269};
​
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.set_l1_l2_collection_mapping(collection_l1, erc721b_address);
stop_prank(CheatTarget::One(bridge_address));
​
spy.fetch_events();
let (_, event) = spy.events.at(0);
let mut req_content = event.data.span();
let mut req = Serde::<Request>::deserialize(ref req_content).unwrap();
req.owner_l2 = owner_l2;
req.collection_l1 = collection_l1;
req.collection_l2 = erc721b_address;
let mut buf = array![];
req.serialize(ref buf);
​
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.enable_white_list(true);
stop_prank(CheatTarget::One(bridge_address));
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(erc721b_address, false); // THIS IS WHERE WHITELISTING IS SET TO FALSE
stop_prank(CheatTarget::One(bridge_address));
​
let mut l1_handler = L1Handler {
contract_address: bridge_address,
// selector: 0x03593216f3a8b22f4cf375e5486e3d13bfde9d0f26976d20ac6f653c73f7e507,
function_selector: selector!("withdraw_auto_from_l1"),
from_address: BRIDGE_L1.into(),
payload: buf.span()
};
l1_handler.execute().unwrap();
assert!(!bridge.is_white_listed(erc721b_address), "should not be whitelisted");
assert!(erc721.owner_of(0) == owner_l2, "Wrong owner");
}

Running snforge test withdraw_token_with_mapping for the test above - shows the test passes and confirms the finding.

Impact

The impact can be high if, for example, a collection was removed from the whitelist because it was compromised. The withdraw_auto_from_l1 will then interact with a compromised contract through the transfer_from function which can cause arbitrary damage/loss of NFT for the user.

Tools Used

Manual review and snforge test

Recommendations

Having a simple check assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted'); after getting the collection_l2 address from ensure_erc721_deployment function would ensure that transfer cannot be made to a non-whitelisted collection. This will revert the transaction and message from L1 would remain unconsumed. The user would then need to cancel the transaction on L1 side to recover the NFT from escrow.

Updates

Lead Judging Commences

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

Support

FAQs

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