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

Wrong access control in L2 `bridge::withdraw_auto_from_l1(..)` function

audinarey

Summary

the bridge::withdraw_auto_from_l1(..)function can be called from any L1 contract due to wrong access control implementation contrary to the intended functionality which is that it can only be called from the Starklane L1 contract.

Vulnerability Details

The bridge::withdraw_auto_from_l1(..)function is used to process withdrawal of tokens deposited on L1 from L2 and must be called only by Starklane L1 contract. However, due to wrong access control, the function can be called by any L1 or L2 contract

File: bridge.cairo
// `from_address` - L1 sender address, must be Starklane L1
130: fn withdraw_auto_from_l1(
131: ref self: ContractState, //@audit this is user crafted input
132: @> from_address: felt252,
133: req: Request
134: ) {
135: ensure_is_enabled(@self); // @audit only checks if bride is not paused (enabled)
136: @> assert(self.bridge_l1_address.read().into() == from_address,
137: 'Invalid L1 msg sender');
138:
139: // TODO: recompute HASH to ensure data are not altered.
140: // TODO: Validate all fields the request (cf. FSM).
141:
142: let collection_l2 = ensure_erc721_deployment(ref self, @req);
143:
144: let _ctype = collection_type_from_header(req.header);
145: // TODO: check CollectionType to support ERC1155 + metadata.
146:
147: let mut i = 0;
​

As shown above, from_address which is used to validate the source address of the caller is user controlled and as such, any malicious L1 contract can call withdraw_auto_from_l1(...)by specifying from_address= EthAddressand the call will pass the check on L136 without reverting.

Impact

Any L1 or L2 contract can call bridge::withdraw_auto_from_l1(..)function contrary to the intended restriction which is that it must be called by a Starklane L1 contract. This break core protocol invariant and functionality.

/// `from_address` - L1 sender address, must be Starklane L1.

Tools Used

Manual review

Recommendations

Modify the bridge::withdraw_auto_from_l1(..)function as shown below

File: bridge.cairo
130: fn withdraw_auto_from_l1(
131: ref self: ContractState,
-132: from_address: felt252, //@audit this is user crafted input
133: req: Request
134: ) {
135: ensure_is_enabled(@self); // @audit only checks if bride is not paused (enabled)
-136: @> assert(self.bridge_l1_address.read().into() == from_address,
-137: 'Invalid L1 msg sender');
+136: @> assert(self.bridge_l1_address.read().into() == get_contract_address(),
+137: 'Invalid L1 msg sender');
138:
139: // TODO: recompute HASH to ensure data are not altered.
140: // TODO: Validate all fields the request (cf. FSM).
141:
142: let collection_l2 = ensure_erc721_deployment(ref self, @req);
143:
144: let _ctype = collection_type_from_header(req.header);
145: // TODO: check CollectionType to support ERC1155 + metadata.
146:
147: let mut i = 0;
​
Updates

Lead Judging Commences

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

Support

FAQs

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