6.1 Potential for double minting via multiple signature sets
When a user deposits on the Bitcoin chain, an approved signer needs to call set_status_pending(...), and after some block confirmations, they'll need to call set_status_funded(...). Both of these functions take signatures of attestors as arguments. These signatures are validated using _attestor_multisig_is_valid(...). It ensures that the number of signatures is greater than or equal to threshold and signatures from the same attestors are not included more than once.
fn _attestor_multisig_is_valid(
ref self: ContractState,
uuid: u256,
btc_tx_id: u256,
tx_type: felt252,
amount: u256,
signatures: Span<(ContractAddress, Array<felt252>)>
) {
let threshold = self.threshold.read();
assert(signatures.len() >= threshold.into(), Errors::NOT_ENOUGH_SIGNATURES);
for (attestor, signature) in signatures {
// ...
let is_valid_signature = IAccountDispatcher { contract_address: *attestor }
.is_valid_signature(message, signature.clone());
assert(is_valid_signature == 'VALID', Errors::INVALID_SIGNATURE);
assert(self.accesscontrol.has_role(APPROVED_SIGNER, *attestor), Errors::INVALID_SIGNER);
self._check_signer_unique(*attestor, message);
}
}
fn _check_signer_unique(
ref self: ContractState,
attestor_pub_key: ContractAddress,
message_hash: felt252,
) {
assert(!self.seen_signers.read((attestor_pub_key, message_hash)), Errors::DUPLICATE_SIGNATURE);
self.seen_signers.write((attestor_pub_key, message_hash), true);
}
However, if there are more attestors than threshold, then a malicious approved signer can execute set_status_pending(...) and set_status_funded(...) more than once by passing different sets of signatures that are at least equal to the threshold.
For example, if threshold is 3 and there are 6 attestors, then a malicious approved signer can execute set_status_pending(...) and set_status_funded(...) via signatures from exactly 3 attestors and then execute both of these again after withdrawing via signatures from 3 different attestors.
Recommendation
Consider adding more checks to ensure that a user's deposit can't be processed more than once. This can be done by adding more validation to the Bitcoin transaction ID.
6.2 The ATTESTOR_MULTISIG_STRUCT_TYPE_HASH includes the u256 struct
The hash of struct like StarknetDomain, AttestorMultisigTx and u256 is calculated using selector macro.
struct StarknetDomain {
name: felt252,
version: felt252,
chain_id: felt252,
revision: felt252,
}
const STARKNET_DOMAIN_TYPE_HASH: felt252 =
selector!(
"\"StarknetDomain\"(\"name\":\"shortstring\",\"version\":\"shortstring\",\"chainId\":\"shortstring\",
\"revision\":\"shortstring\")"
);
pub struct AttestorMultisigTx {
uuid: u256,
btc_tx_id: u256,
tx_type: felt252,
amount: u256,
}
// @audit it also contains fields of u256
const ATTESTOR_MULTISIG_STRUCT_TYPE_HASH: felt252 =
selector!("\"AttestorMultisigTx\"(\"uuid\":\"u256\",\"btc_tx_id\":\"u256\",\"tx_type\":\"felt\",\"amount\":
\"u256\")\"u256\"(\"low\":\"u128\",\"high\":\"u128\")");
const U256_TYPE_HASH: felt252 = selector!("\"u256\"(\"low\":\"u128\",\"high\":\"u128\")");
However, the ATTESTOR_MULTISIG_STRUCT_TYPE_HASH incorrectly contains a u256 struct at the end.
Recommendation
Consider removing the u256 struct in the hash of the AttestorMultisigTx struct.
6.3 The ibtc_manager does not have a function to upgrade ibtc_token contract
The ibtc_token contract has an upgrade(...) function that can only be executed by the owner. The owner for the ibtc_token contract is the ibtc_manager. However, there's no function in the ibtc_manager contract to execute upgrade(...) in the ibtc_token contract.
Although transfer_token_contract_ownership(...) can be used to transfer ownership, the new owner can then execute upgrade(...) in the ibtc_token contract.
Recommendation
Consider adding a function in the ibtc_manager contract to execute upgrade(...) in the ibtc_token contract.
6.4 The set_approved_signers(...) override signer_count rather than increasing it
When an account is granted APPROVED_SIGNER role via grant_role(...), the signer_count is increase by 1.
fn grant_role(ref self: ContractState, role: felt252, account: ContractAddress) {
if (self._has_any_roles(account)) {
panic_with_felt252(Errors::INCOMPATIBLE_ROLE);
}
self.accesscontrol.grant_role(role, account);
if (role == APPROVED_SIGNER) {
self.signer_count.write(self.signer_count.read() + 1);
}
}
However, an account can be granted the APPROVED_SIGNER role using set_approved_signers(...), too. However, this doesn't increase the signer_count but rather overrides it.
fn set_approved_signers(ref self: ContractState, signers: Array<ContractAddress>) {
self.accesscontrol.assert_only_role(IBTC_ADMIN_ROLE);
let signers_len = signers.len();
let mut i: usize = 0;
loop {
if i >= signers_len {
break;
}
let signer = signers.at(i);
self.accesscontrol._grant_role(APPROVED_SIGNER, *signer);
i += 1;
};
// @audit signer_count is overriden
self.signer_count.write(signers_len.try_into().unwrap());
self.emit(SetApprovedSigners{signers});
}
If set_approved_signers(...) is ever used after executing grant_role(...) to grant an account the APPROVED_SIGNER role, the signer_count would have incorrect accounting.
Recommendation
Consider increasing signer_count rather than overriding it in set_approved_signers(...).
Update from Nethermind Security
The set_approved_signers(...) can only be executed by IBTC_ADMIN_ROLE. Previously, it was directly using _grant_role(...) which doesn't have any access control so the caller of set_approved_signers(...) needs to have IBTC_ADMIN_ROLE only. However, now, it further invokes grant_role(...) which can only be executed by admin of that role i.e. admin of APPROVED_SIGNER role i.e. DEFAULT_ADMIN_ROLE. So the caller not only needs to have IBTC_ADMIN_ROLE but also DEFAULT_ADMIN_ROLE.
6.5 The set_status_funded(...) can be executed for a different transaction ID than that of set_status_pending(...)
When a user deposits on the Bitcoin chain, an approved signer needs to call set_status_pending(...), and after some block confirmations, they'll need to call set_status_funded(...). Both of these functions take a transaction ID as an argument. The transaction ID is expected to be the same in both cases. However, the code doesn't ensure that the transaction ID passed as an argument in set_status_funded(...) is the same as the one passed in set_status_pending(...).
Recommendation
Consider adding a check to ensure that the transaction ID passed in set_status_funded(...) is the same as the one passed in set_status_pending(...).
6.6 The _check_por(...) does not need to return bool
The _check_por(...) is not completely implemented yet, but it's expected to either always return true or revert. If it returns false, then the iBTC won't be minted to the user for their deposits, and their deposit on the Bitcoin chain would be stuck. So, it should revert rather than return false, as also done in the Solidity implementation. As the function will never return false, there's no need to return a boolean.
Recommendation
Consider updating the function signature for _check_por(...).
Update from Nethermind Security
The _check_mint(...) is also modified and now it doesn't return the bool which completely defeats its purpose. It was previously used in if check to ensure that tokens are only minted if it returns true, whereas now as it doesn't return a bool it's executed and not used in if check and tokens are minted irrespectively. Thus, now executing _check_mint(...) does nothing.
6.7 The get_ssf_message(...) and get_ssp_message(...) takes mutable reference of the contract state
The get_ssf_message(...) and get_ssp_message(...) functions return the hash of the message that can be used by attestors to sign the message. It doesn't update the contract state but just returns the calculated hash.
#[external(v0)]
fn get_ssf_message(ref self: ContractState, attestor: ContractAddress, uuid: u256, btc_tx_id: u256, new_value_locked: u256) -> felt252 {
let message = AttestorMultisigTx {
uuid,
btc_tx_id,
tx_type: 'set-status-funded',
amount: new_value_locked,
};
message.get_message_hash(attestor)
}
#[external(v0)]
fn get_ssp_message(ref self: ContractState, attestor: ContractAddress, uuid: u256, wdtx_id: u256, new_value_locked: u256) -> felt252 {
let message = AttestorMultisigTx {
uuid,
btc_tx_id: wdtx_id,
tx_type: 'set-status-pending',
amount: new_value_locked,
};
let message_hash = message.get_message_hash(attestor);
message_hash
}
However, both functions take a mutable reference of self, i.e, the contract state.
Recommendation
Consider taking a snapshot of the contract state ( self: @ContractState) rather than a mutable reference ( ref self: ContractState).
