Summary
Setting use_withdraw_auto to true in L2 -> L1 bridging will lead to permanent loss of NFTs.
Vulnerability Details
To bridge NFTs from L2 -> L1, users can use the deposit_tokens() function of bridge.cairo :
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');
// is not giving approval to the bridge.
let from = starknet
assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted');
let ctype = CollectionType::ERC721;
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids))
let (name, symbol, base_uri, uris) = match erc721_metadata {
Option::Some(data) => (data.name, data.symbol, data.base_uri, data.uris),
Option::None => ("", "", "", array![].span())
}
escrow_deposit_tokens(ref self, collection_l2, from, token_ids);
let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
let req = Request {
@=> header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
collection_l1,
collection_l2,
owner_l1,
owner_l2: from,
name,
symbol,
base_uri,
ids: token_ids,
values: array![].span(),
uris,
new_owners: array![].span(),
}
let mut buf: Array<felt252> = array![];
req.serialize(ref buf);
starknet::send_message_to_l1_syscall(
self.bridge_l1_address.read().into(),
buf.span(),
)
.unwrap_syscall();
self.emit(DepositRequestInitiated {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
})
}
The use_withdraw_auto parameter is user-controlled and can be set to either true or false by the user. As you can see, the value of this bool is not validated anywhere in the function and it is simply appended to the header of the L2 to L1 Request.
On the L1 bridge, the withdrawTokens() function is used to withdraw the NFTs from the L2 -> L1 bridge request :
function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
uint256 header = request[0];
@=> if (Protocol.canUseWithdrawAuto(header)) {
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
Request memory req = Protocol.requestDeserialize(request, 0);
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
if (!wasEscrowed) {
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}
emit WithdrawRequestCompleted(req.hash, block.timestamp, request);
return collectionL1;
}
Here, you can see that the header of the Request is validated through the Protocol.canUseWithdrawAuto(header) function, which checks the value of the use_withdraw_auto bool that the user set in the L2 deposit_tokens() function.
The withdraw auto functionality was removed from the function after a critical vulnerability was found in a previous audit. Hence, the function now reverts if use_withdraw_auto is set to true. However, this will cause any request that was made with use_withdraw_auto set to true to revert, and the NFTs that were supposed to be withdrawn from the L1 bridge to be stuck forever as there is no way to retrieve them back on L2 bridge.
Impact
Permanent loss of assets
Tools Used
Manual Review
Recommendations
Enforce use_withdraw_autoto be set to false in bridge.cairo::deposit_tokens():
- header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
+ header: compute_request_header_v1(ctype, use_deposit_burn_auto, false),
