6.1 Missing token balance checks on ERC20 transfer functions
The token_bridge contract allows users to withdraw tokens through several functions: withdraw(...), deposit_with_message_reclaim(...), and deposit_reclaim(...). In all these cases, the function first consumes a message to authorize the transfer and then proceeds to send the tokens to the recipient.
The issue is that these functions do not verify whether the token transfer was successful. The message is consumed optimistically before the transfer(...) call is made. Some ERC20 tokens may not revert on a failed transfer (e.g., if the token is paused or the recipient is blacklisted) but instead return a boolean status. Since this return value is not checked, the transaction can succeed even if the tokens were not actually moved.
This can lead to a scenario where a user's withdrawal message is consumed, but they never receive their tokens due to a failed transfer. Because the message is marked as used, the user cannot retry the operation, resulting in their funds being permanently stuck in the bridge contract. It is worth noting that the accept_deposit(...) function correctly checks for transfer success by comparing balances before and after the call, but this pattern was not applied to the withdrawal and reclaim flows.
For brevity, only the withdraw(...) function is shown, as the reclaim functions follow the same flawed logic.
fn withdraw(
ref self: ContractState,
token: ContractAddress,
amount: u256,
recipient: ContractAddress,
) {
// ...
// @audit The message is consumed before the transfer is attempted.
self.consume_message(token, amount, recipient);
let tokenDispatcher = IERC20Dispatcher { contract_address: token };
// @audit-issue The success of this transfer is not verified.
// If it fails, the message is already consumed and funds are lost.
tokenDispatcher.transfer(recipient, amount);
// ...
}
Recommendation
Consider validating that the token transfer was successful in the withdraw(...), deposit_with_message_reclaim(...), and deposit_reclaim(...) functions. This can be achieved similarly to the implementation in the accept_deposit(...) function.
6.2 Inconsistent ERC20 Standard Usage
The accept_deposit(...) function is responsible for handling user deposits of ERC20 tokens into the contract. It verifies that the token is supported, checks against a maximum balance, and then performs a transfer_from(...) operation to move tokens from the caller to the contract's address. Finally, it asserts that the transfer was successful by comparing the contract's balance before and after the transfer.
The issue lies in the fact that Starknet ERC20 tokens can have different function signatures, specifically camelCase (e.g., transferFrom(...), balanceOf(...)) or snake_case (e.g., transfer_from(...), balance_of(...)). While the latest standard recommends using snake_case, older tokens might still adhere to camelCase. The current implementation exclusively uses snake_case for balance_of(...) and transfer_from(...) calls.
fn accept_deposit(self: @ContractState, token: ContractAddress, amount: u256) {
// ...
let dispatcher = IERC20Dispatcher { contract_address: token };
// @audit Using snake_case
let current_balance: u256 = dispatcher.balance_of(get_contract_address());
let max_total_balance = self.get_max_total_balance(token);
assert(current_balance + amount < max_total_balance, Errors::MAX_BALANCE_EXCEEDED);
let this_address = get_contract_address();
// @audit Using snake_case
let initial_balance = dispatcher.balance_of(this_address);
// @audit-issue Only snake_case `transfer_from(...)` is used.
dispatcher.transfer_from(caller, this_address, amount);
assert(
// @audit Using snake_case
dispatcher.balance_of(this_address) == initial_balance + amount,
Errors::TOKENS_NOT_TRANSFERRED,
);
}
If a user attempts to deposit an ERC20 token that uses camelCase function signatures, the transfer_from(...) call will fail. This is because the dispatcher is configured to call the snake_case version of the function, which does not exist on camelCase standard tokens.
Recommendation
Consider implementing a mechanism to support both camelCase and snake_case ERC20 token standards. This would ensure compatibility with a wider range of ERC20 tokens on Starknet.
Resolution
We acknowledge this issue. We feel this should be fine as most of the relevant tokens on Starknet mainnet have already migrated, and going for forward the new standard says the token must have snake_case. On a side note madara currently supports cairo version 2.9.2 which does not have try/catch. This can be revisited in the next sprint when we migrate to higher cairo version.
6.3 Incorrect max balance exceeded assertion in the accept_deposit(...) function
In the accept_deposit(...) function, the contract checks if a new deposit would exceed the max_total_balance for a token. The check is performed using a strict less-than ( <) comparison.
This prevents a deposit that would make the contract's balance exactly equal to max_total_balance. This behavior is inconsistent with the reference Starkgate implementation, which uses a less-than-or-equal-to ( <=) check, allowing the balance to reach the maximum limit.
fn accept_deposit(self: @ContractState, token: ContractAddress, amount: u256) {
// ...
let current_balance = IERC20Dispatcher { contract_address: token }.balance_of(self.own_address);
let max_total_balance = self.token_settings.read(token).max_total_balance;
// @audit-issue The use of `<` prevents the total balance from ever reaching max_total_balance.
assert(current_balance + amount < max_total_balance, Errors::MAX_BALANCE_EXCEEDED);
// ...
}
Recommendation
Consider changing the strict less-than ( <) comparison to less-than-or-equal-to ( <=) to correctly allow deposits up to the maximum configured balance.
6.4 System does not support ERC20 tokens that lack the optional decimals function
The deployment_message_payload(...) function constructs a calldata payload containing metadata about a given token. This metadata includes the token's name, symbol, and the number of decimals. The function makes a direct external call to the decimals() function on the token's contract address to fetch this information.
pub fn deployment_message_payload(token: ContractAddress) -> Span<felt252> {
// ...
let dispatcher = IERC20MetadataDispatcher { contract_address: token };
token.serialize(ref calldata);
// ...
let symbol_selector = selector!("symbol");
let mut symbol = call_contract_syscall(token, symbol_selector, array![].span())
.unwrap_syscall();
calldata = deserialize_and_append(symbol, calldata);
// @audit-issue The decimals() function is called directly, but it is optional in the ERC20 standard.
dispatcher.decimals().serialize(ref calldata);
calldata.span()
}
The problem arises because the ERC20 token standard specifies that the decimals() function is OPTIONAL. While many tokens implement it for usability, its presence is not guaranteed.
Consequently, any valid ERC20-compliant token that does not implement the decimals() function will be incompatible with the protocol. When the deployment_message_payload(...) function is called with such a token, the direct call to dispatcher.decimals() will fail, causing the transaction to revert. This restricts the variety of tokens the platform can support and may lead to unexpected failures for users.
Recommendation
Consider whether the decimals() value is strictly necessary for the deployment_message_payload(...) function's intended purpose. If the decimals() information is not critical for the core functionality, consider removing the call to dispatcher.decimals() to ensure broader compatibility with all ERC20 tokens, including those that do not implement the optional decimals() function.
Resolution
Acknowledged. We decide on to support tokens that support decimals().
6.5 The initiate_token_withdraw_with_id function usage is not enforced and ID uniqueness is not validated
The TokenBridge contract facilitates bridging tokens from L3 to the Starknet L2. A new function, initiate_token_withdraw_with_id(...), was introduced to help off-chain services track withdrawal transactions. This function takes a user-supplied id, calls the original initiate_token_withdraw(...) function, and then emits a WithdrawInitiatedWithId event that includes this id.
However, the original initiate_token_withdraw(...) function remains publicly callable. This allows users to initiate a withdrawal by calling it directly, which will only emit the WithdrawInitiated event, bypassing the new event with the unique id. Additionally, the id parameter in the new initiate_token_withdraw_with_id(...) function is provided by the user, and the contract does not enforce its uniqueness.
This design has two implications for off-chain systems. First, any indexing service that relies exclusively on the WithdrawInitiatedWithId event will fail to track withdrawals made through direct calls to initiate_token_withdraw(...). Second, since the uniqueness of the id is not guaranteed on-chain, different withdrawal transactions could be emitted with the same ID, which the off-chain tracking logic must be designed to handle correctly.
The following code snippets show the two functions:
fn initiate_token_withdraw(
ref self: ContractState,
l1_token: ContractAddress,
l1_recipient: ContractAddress,
amount: u256,
) {
// ...
// @audit This function is still callable directly.
self.emit(WithdrawInitiated { l1_token, l1_recipient, amount, caller_address });
}
The newly added initiate_token_withdraw_with_id(...) function calls the original initiate_token_withdraw(...) function and emits an additional event.
fn initiate_token_withdraw_with_id(
ref self: ContractState,
l1_token: ContractAddress,
l1_recipient: ContractAddress,
amount: u256,
id: u256, // @audit-issue The uniqueness of this ID is not enforced.
) {
self
.initiate_token_withdraw(
l1_token: l1_token, l1_recipient: l1_recipient, amount: amount,
);
self
.emit(
WithdrawInitiatedWithId {
l1_token, l1_recipient, amount, caller_address: get_caller_address(), id,
},
);
}
Recommendation
Consider if the consistency and uniqueness of withdrawal tracking are critical enough to be enforced on-chain. If correctness is crucial, changes could be implemented to ensure all withdrawals utilize the ID-based event mechanism and that the provided IDs are unique. Otherwise, the off-chain logic can be designed to handle both withdrawal paths and potential duplicate IDs to enforce correctness.
Resolution
Acknowledged. The off-chain logic will manage the correctness.
6.6 Token activation can fail due to a race condition
To enable deposits for a new token, it must first be deployed on the Appchain. This process is initiated on Starknet, and the token's status in the token_bridge contract is set to Pending. The check_deployment_status(...) function is then called within deposit-related functions to check if the deployment message has been successfully consumed on the Appchain and verified on Starknet. If so, the message is marked as SEALED and the token's status is updated to Active, enabling deposits.
The check_deployment_status(...) function includes a timeout mechanism. If a Pending token's deployment message is not SEALED within the pending_deployment_expiration period (currently 5 days), its status is reverted to Unknown. A race condition can occur if an Appchain's sequencer is inactive for more than 5 days and then comes back online to process the deployment message.
In this scenario, the token will be successfully deployed on the Appchain, but its status on the Starknet token_bridge will have already reverted to Unknown. Any subsequent attempts to re-register the token will fail on the Appchain with a TOKEN_ALREADY_EXISTS error. Since there is no mechanism to remove the token on the Appchain, this leads to a permanent state inconsistency, effectively blocking all future deposits for that token through the bridge. While this is unlikely for the main Starknet L1 -> L2 bridge, it poses a risk for newer Appchains that may experience sequencer downtime.
fn check_deployment_status(ref self: ContractState, token: ContractAddress) {
self.pausable.assert_not_paused();
let settings = self.token_settings.read(token);
if (settings.token_status != TokenStatus::Pending) {
return;
}
let message_status = self
.messaging_contract
.read()
.sn_to_appchain_messages(settings.deployment_message_hash);
if (message_status == MessageToAppchainStatus::Sealed) {
let new_settings = TokenSettings { token_status: TokenStatus::Active, ..settings };
self.token_settings.write(token, new_settings);
self.emit(TokenActivated { token });
} else if (get_block_timestamp() > settings.pending_deployment_expiration) {
let new_settings = TokenSettings { token_status: TokenStatus::Unknown, ..settings };
// @audit-issue If the Appchain processes the deployment after this timeout, the token
// will be deployed on the Appchain but remain `Unknown` on Starknet, blocking it permanently.
self.token_settings.write(token, new_settings);
}
}
Recommendation
Consider revisiting the token activation mechanism to handle this edge case. One approach could be to utilize the Piltover's message cancellation mechanism to ensure that the token wasn't activated at the smart contract level.
Update from Nethermind Security
The latest changes, contain issues that need to be addressed:
- The condition
message_status != MessageToAppchainStatus::Cancelling && get_block_timestamp() > settings.pending_deployment_expiration is problematic. This logic allows the code to enter this branch even if the message has already been cancelled, preventing it from reaching the MessageToAppchainStatus::Cancelled handling. The start_message_cancellation function should only be invoked when the message status is Pending; - The current implementation is missing a crucial step. After
start_message_cancellation is called and the cancellation delay has passed, there should be a call to the cancel_message function. Without this, the message cancellation process remains incomplete;
6.7 Token settings are not cleared upon unsuccessful token activation
The token_bridge contract manages the enrollment of new tokens. When a user interacts with a token, the check_deployment_status(...) function is called to verify if the token has been successfully activated on the Appchain. Activation is confirmed if the corresponding deployment message from Starknet has been sealed.
If the deployment message is not sealed within the pending_deployment_expiration window (currently 5 days), the token activation is considered to have failed. In this scenario, the function correctly reverts the token_status to TokenStatus::Unknown. However, it fails to clear the deployment_message_hash and pending_deployment_expiration from the failed enrollment attempt.
While other settings like max_total_balance may be intended to persist as they can be set by the governor independently of the token status, the leftover message hash and expiration time represent stale data. This "dirty" state could be misleading for off-chain tooling that queries these values and could introduce subtle bugs in future contract iterations if new logic does not account for an Unknown token having leftover deployment data.
fn check_deployment_status(ref self: ContractState, token: ContractAddress) {
// ...
if (message_status == MessageToAppchainStatus::Sealed) {
// ...
} else if (get_block_timestamp() > settings.pending_deployment_expiration) {
// @audit-issue Only the token_status is updated to Unknown. Other settings from the failed
// deployment, like deployment_message_hash and pending_deployment_expiration,
// persist, leaving stale data in storage.
let new_settings = TokenSettings { token_status: TokenStatus::Unknown, ..settings };
self.token_settings.write(token, new_settings);
}
}
Recommendation
Consider clearing all relevant settings for a token when its deployment expires and it is marked as Unknown.
