Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treasury Audit Fixes #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions contracts/account/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ pub enum ContractError {

#[error(transparent)]
FromUTF8(#[from] std::string::FromUtf8Error),

#[error("Not found: {msg}")]
NotFound { msg: String },
}

pub type ContractResult<T> = Result<T, ContractError>;
Expand Down
10 changes: 10 additions & 0 deletions contracts/account/src/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ pub fn save_authenticator(
}

pub fn remove_auth_method(deps: DepsMut, env: Env, id: u8) -> ContractResult<Response> {
// Ensure there are more than 1 authenticator before removing
if AUTHENTICATORS
.keys(deps.storage, None, None, Order::Ascending)
.count()
Expand All @@ -250,7 +251,16 @@ pub fn remove_auth_method(deps: DepsMut, env: Env, id: u8) -> ContractResult<Res
return Err(ContractError::MinimumAuthenticatorCount);
}

// Validate that the key exists
if !AUTHENTICATORS.has(deps.storage, id) {
return Err(ContractError::NotFound {
msg: format!("Authenticator ID {} does not exist", id),
});
}

// Remove the authenticator
AUTHENTICATORS.remove(deps.storage, id);

Ok(
Response::new().add_event(Event::new("remove_auth_method").add_attributes(vec![
("contract_address", env.contract.address.to_string()),
Expand Down
16 changes: 13 additions & 3 deletions contracts/treasury/src/contract.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::error::ContractResult;
use crate::error::{ContractError, ContractResult};
use crate::execute::{revoke_allowance, update_fee_config, update_params, withdraw_coins};
use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg};
use crate::{execute, query, CONTRACT_NAME, CONTRACT_VERSION};
Expand All @@ -14,10 +14,16 @@ pub fn instantiate(
msg: InstantiateMsg,
) -> ContractResult<Response> {
cw2::set_contract_version(deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?;
// Validate the admin address
let admin_addr = if let Some(addr) = msg.admin {
deps.api.addr_validate(&addr.as_str())?
} else {
return Err(ContractError::Unauthorized);
};
execute::init(
deps,
info,
msg.admin,
Some(admin_addr),
msg.type_urls,
msg.grant_configs,
msg.fee_config,
Expand All @@ -36,7 +42,11 @@ pub fn execute(
authz_granter,
authz_grantee,
} => execute::deploy_fee_grant(deps, env, authz_granter, authz_grantee),
ExecuteMsg::UpdateAdmin { new_admin } => execute::update_admin(deps, info, new_admin),
ExecuteMsg::ProposeAdmin { new_admin } => {
execute::propose_admin(deps, info, new_admin.into_string())
}
ExecuteMsg::AcceptAdmin {} => execute::accept_admin(deps, info),
ExecuteMsg::CancelProposedAdmin {} => execute::cancel_proposed_admin(deps, info),
ExecuteMsg::UpdateGrantConfig {
msg_type_url,
grant_config,
Expand Down
3 changes: 3 additions & 0 deletions contracts/treasury/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ pub enum ContractError {

#[error("unauthorized")]
Unauthorized,

#[error("Not found: {msg}")]
NotFound { msg: String },
}

pub type ContractResult<T> = Result<T, ContractError>;
72 changes: 65 additions & 7 deletions contracts/treasury/src/execute.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::error::ContractError::{
AuthzGrantMismatch, AuthzGrantNotFound, ConfigurationMismatch, Unauthorized,
AuthzGrantMismatch, AuthzGrantNotFound, ConfigurationMismatch, NotFound, Unauthorized,
};
use crate::error::ContractResult;
use crate::grant::allowance::format_allowance;
use crate::grant::{FeeConfig, GrantConfig};
use crate::state::{Params, ADMIN, FEE_CONFIG, GRANT_CONFIGS, PARAMS};
use crate::state::{Params, ADMIN, FEE_CONFIG, GRANT_CONFIGS, PARAMS, PENDING_ADMIN};
use cosmos_sdk_proto::cosmos::authz::v1beta1::{QueryGrantsRequest, QueryGrantsResponse};
use cosmos_sdk_proto::cosmos::feegrant::v1beta1::QueryAllowanceRequest;
use cosmos_sdk_proto::prost::Message;
Expand Down Expand Up @@ -46,22 +46,71 @@ pub fn init(
))
}

pub fn update_admin(deps: DepsMut, info: MessageInfo, new_admin: Addr) -> ContractResult<Response> {
pub fn propose_admin(
deps: DepsMut,
info: MessageInfo,
new_admin: String,
) -> ContractResult<Response> {
// Load the current admin
let admin = ADMIN.load(deps.storage)?;

// Check if the caller is the current admin
if admin != info.sender {
return Err(Unauthorized);
}

ADMIN.save(deps.storage, &new_admin)?;
// Validate the new admin address
let validated_admin = deps.api.addr_validate(&new_admin)?;

// Save the proposed new admin to PENDING_ADMIN
PENDING_ADMIN.save(deps.storage, &validated_admin)?;

Ok(
Response::new().add_event(Event::new("updated_treasury_admin").add_attributes(vec![
("old admin", admin.into_string()),
("new admin", new_admin.into_string()),
Response::new().add_event(Event::new("proposed_new_admin").add_attributes(vec![
("proposed_admin", validated_admin.to_string()),
("proposer", admin.to_string()),
])),
)
}

pub fn accept_admin(deps: DepsMut, info: MessageInfo) -> ContractResult<Response> {
// Load the pending admin
let pending_admin = PENDING_ADMIN.load(deps.storage)?;

// Verify the sender is the pending admin
if pending_admin != info.sender {
return Err(Unauthorized);
}

// Update the ADMIN storage with the new admin
ADMIN.save(deps.storage, &pending_admin)?;

// Clear the PENDING_ADMIN
PENDING_ADMIN.remove(deps.storage);

Ok(Response::new().add_event(
Event::new("accepted_new_admin")
.add_attributes(vec![("new_admin", pending_admin.to_string())]),
))
}

pub fn cancel_proposed_admin(deps: DepsMut, info: MessageInfo) -> ContractResult<Response> {
// Load the current admin
let admin = ADMIN.load(deps.storage)?;

// Check if the caller is the current admin
if admin != info.sender {
return Err(Unauthorized);
}

// Remove the pending admin
PENDING_ADMIN.remove(deps.storage);

Ok(Response::new().add_event(
Event::new("cancelled_proposed_admin").add_attribute("action", "cancel_proposed_admin"),
))
}

pub fn update_grant_config(
deps: DepsMut,
info: MessageInfo,
Expand Down Expand Up @@ -90,11 +139,20 @@ pub fn remove_grant_config(
info: MessageInfo,
msg_type_url: String,
) -> ContractResult<Response> {
// Check if the sender is the admin
let admin = ADMIN.load(deps.storage)?;
if admin != info.sender {
return Err(Unauthorized);
}

// Validate that the key exists
if !GRANT_CONFIGS.has(deps.storage, msg_type_url.clone()) {
return Err(NotFound {
msg: format!("Grant config for '{}' does not exist", msg_type_url),
});
}

// Remove the grant config
GRANT_CONFIGS.remove(deps.storage, msg_type_url.clone());

Ok(Response::new().add_event(
Expand Down
4 changes: 3 additions & 1 deletion contracts/treasury/src/msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@ pub struct InstantiateMsg {

#[cw_serde]
pub enum ExecuteMsg {
UpdateAdmin {
ProposeAdmin {
new_admin: Addr,
},
AcceptAdmin {},
CancelProposedAdmin {},
UpdateGrantConfig {
msg_type_url: String,
grant_config: GrantConfig,
Expand Down
2 changes: 2 additions & 0 deletions contracts/treasury/src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ pub const FEE_CONFIG: Item<FeeConfig> = Item::new("fee_config");

pub const ADMIN: Item<Addr> = Item::new("admin");

pub const PENDING_ADMIN: Item<Addr> = Item::new("pending_admin");

#[cw_serde]
pub struct Params {
pub display_url: String,
Expand Down
Loading