Bypass overriding index
Description
The function add_auth_method is used to add a new authentication method to the account, and the function save_authenticator is used to save AUTHENTICATORS with checking the index is not already used to avoid overriding the existing authenticator. However, the AddAuthenticator::Secp256R1 case in add_auth_method does not call save_authenticator to save the authenticator, and it directly saves the authenticator to storage.
pub fn add_auth_method(
deps: DepsMut,
env: &Env,
add_authenticator: &mut AddAuthenticator,
) -> ContractResult<Response> {
match add_authenticator.borrow_mut() {
// ...
AddAuthenticator::Jwt {
id,
aud,
sub,
token,
} => {
// ...
save_authenticator(deps, *id, &auth)?;
Ok(())
}
AddAuthenticator::Secp256R1 {
id,
pubkey,
signature,
} => {
let auth = Authenticator::Secp256R1 {
pubkey: (*pubkey).clone(),
};
if !auth.verify(
deps.as_ref(),
env,
&Binary::from(env.contract.address.as_bytes()),
signature,
)? {
Err(ContractError::InvalidSignature)
} else {
AUTHENTICATORS.save(deps.storage, *id, &auth)?;
Ok(())
}
}
AddAuthenticator::Passkey {
id,
url,
credential,
} => {
// ...
save_authenticator(deps, *id, &auth)?;
// ...
Ok(())
}
}?;
//...
}
pub fn save_authenticator(
deps: DepsMut,
id: u8,
authenticator: &Authenticator,
) -> ContractResult<()> {
if AUTHENTICATORS.has(deps.storage, id) {
return Err(ContractError::OverridingIndex { index: id });
}
AUTHENTICATORS.save(deps.storage, id, authenticator)?;
Ok(())
}Impact
This could lead to overriding the existing authenticator in storage. There is no security impact on the contract, but if both add and remove auth method events occur in a tracking scenario, it could lead to confusion.
Recommendations
Change the AddAuthenticator::Secp256R1 case in add_auth_method to call save_authenticator to save the authenticator.
Remediation
This issue has been acknowledged by Burnt Labs, and a fix was implemented in commit 0e1583eb↗.