Skip to content

Commit

Permalink
Merge pull request #5 from skip-mev/jw/address-audit-findings
Browse files Browse the repository at this point in the history
Address audit issues
thal0x authored Nov 6, 2024
2 parents 0258c34 + f12611f commit 5c41bd4
Showing 32 changed files with 1,001 additions and 102 deletions.
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -7,3 +7,6 @@
[submodule "solidity/lib/openzeppelin-contracts-upgradeable"]
path = solidity/lib/openzeppelin-contracts-upgradeable
url = https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable
[submodule "solidity/lib/skip-go-evm-contracts"]
path = solidity/lib/skip-go-evm-contracts
url = https://github.com/skip-mev/skip-go-evm-contracts
8 changes: 8 additions & 0 deletions cosmwasm/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions cosmwasm/Cargo.toml
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ members = [
"contracts/fast-transfer-gateway",
"contracts/cw7683",
"packages/*",
"bin/print-order-id",
]

[profile.release]
8 changes: 8 additions & 0 deletions cosmwasm/bin/print-order-id/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
[package]
name = "print-order-id"
version = "0.1.0"
edition = "2021"

[dependencies]
cosmwasm-std = { workspace = true }
go-fast = { workspace = true }
19 changes: 19 additions & 0 deletions cosmwasm/bin/print-order-id/src/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
use cosmwasm_std::{HexBinary, Uint128};
use go_fast::{helpers::keccak256_hash, FastTransferOrder};

fn main() {
let order = FastTransferOrder {
sender: keccak256_hash("order_sender".as_bytes()),
recipient: keccak256_hash("order_recipient".as_bytes()),
amount_in: Uint128::new(1_000000),
amount_out: Uint128::new(2_000000),
nonce: 5,
source_domain: 1,
destination_domain: 2,
timeout_timestamp: 1234567890,
data: Some(HexBinary::from("order_data".as_bytes())),
};

println!("== Output ==");
println!("{}", order.id());
}
6 changes: 6 additions & 0 deletions cosmwasm/contracts/fast-transfer-gateway/src/error.rs
Original file line number Diff line number Diff line change
@@ -44,6 +44,12 @@ pub enum ContractError {

#[error("Incorrect domain for settlement")]
IncorrectDomainForSettlement,

#[error("Order recipient cannot be mailbox")]
OrderRecipientCannotBeMailbox,

#[error("Duplicate order")]
DuplicateOrder,
}

pub type ContractResult<T> = Result<T, ContractError>;
9 changes: 9 additions & 0 deletions cosmwasm/contracts/fast-transfer-gateway/src/execute.rs
Original file line number Diff line number Diff line change
@@ -61,6 +61,10 @@ pub fn fill_order(

let recipient_address = bech32_encode(&config.address_prefix, &order.recipient)?;

if recipient_address == config.mailbox_addr {
return Err(ContractError::OrderRecipientCannotBeMailbox);
}

let msg: CosmosMsg = match order.data {
Some(data) => WasmMsg::Execute {
contract_addr: recipient_address.clone().to_string(),
@@ -96,6 +100,10 @@ pub fn initiate_settlement(
return Err(ContractError::Unauthorized);
}

if fills_to_settle.contains(&order_fill) {
return Err(ContractError::DuplicateOrder);
}

fills_to_settle.push(order_fill);
}

@@ -145,6 +153,7 @@ pub fn initiate_timeout(
for order in &orders {
assert_order_is_expired(&env, order)?;
assert_order_not_filled(deps.as_ref(), order.id())?;
assert_local_domain(deps.as_ref(), order.destination_domain)?;
}

let order_ids = orders
6 changes: 1 addition & 5 deletions cosmwasm/contracts/fast-transfer-gateway/src/helpers.rs
Original file line number Diff line number Diff line change
@@ -48,12 +48,8 @@ pub fn assert_order_is_expired(env: &Env, order: &FastTransferOrder) -> Contract
}

pub fn assert_order_is_not_expired(env: &Env, order: &FastTransferOrder) -> ContractResult<()> {
if order.timeout_timestamp == 0 {
return Ok(());
}

let timeout_timestamp = Timestamp::from_seconds(order.timeout_timestamp);
if env.block.time.seconds() > timeout_timestamp.seconds() {
if env.block.time.seconds() >= timeout_timestamp.seconds() {
return Err(ContractError::OrderTimedOut);
}

18 changes: 16 additions & 2 deletions cosmwasm/contracts/fast-transfer-gateway/tests/common/mod.rs
Original file line number Diff line number Diff line change
@@ -6,10 +6,12 @@ use cosmwasm_std::{
};
use go_fast::{
gateway::{Config, ExecuteMsg},
helpers::keccak256_hash,
FastTransferOrder,
};
use go_fast_transfer_cw::{
error::ContractResponse,
helpers::bech32_encode,
state::{CONFIG, LOCAL_DOMAIN, NONCE, REMOTE_DOMAINS},
};
use hyperlane::mailbox::{DefaultHookResponse, QueryMsg as HplQueryMsg, RequiredHookResponse};
@@ -28,7 +30,12 @@ pub fn default_instantiate() -> (OwnedDeps<MemoryStorage, MockApi, MockQuerier>,
&Config {
token_denom: "uusdc".to_string(),
address_prefix: "osmo".to_string(),
mailbox_addr: "mailbox_contract_address".into(),
mailbox_addr: bech32_encode(
"osmo",
&keccak256_hash("mailbox_contract_address".as_bytes()),
)
.unwrap()
.into_string(),
hook_addr: "hook_contract_address".into(),
},
)
@@ -50,7 +57,14 @@ pub fn default_instantiate() -> (OwnedDeps<MemoryStorage, MockApi, MockQuerier>,
let wasm_handler = |query: &WasmQuery| -> QuerierResult {
match query {
WasmQuery::Smart { contract_addr, msg } => {
if contract_addr == "mailbox_contract_address" {
if contract_addr
== &bech32_encode(
"osmo",
&keccak256_hash("mailbox_contract_address".as_bytes()),
)
.unwrap()
.into_string()
{
let msg: HplQueryMsg = from_json(msg).unwrap();
match msg {
HplQueryMsg::Hook(_) => todo!(),
Original file line number Diff line number Diff line change
@@ -8,7 +8,10 @@ use go_fast::{
gateway::{ExecuteMsg, OrderFill, QueryMsg},
FastTransferOrder,
};
use go_fast_transfer_cw::helpers::{bech32_decode, left_pad_bytes};
use go_fast_transfer_cw::{
helpers::{bech32_decode, left_pad_bytes},
state::CONFIG,
};

pub mod common;

@@ -167,6 +170,55 @@ fn test_fill_order_with_data() {
);
}

#[test]
fn test_fill_order_fails_when_order_recipient_is_mailbox() {
let (mut deps, env) = default_instantiate();

let user_address = deps.api.with_prefix("osmo").addr_make("user");

let test_payload = to_json_binary(&BankMsg::Send {
to_address: "solver".to_string(),
amount: vec![coin(98_000_000, "uusdc")],
})
.unwrap();

let mailbox_address = CONFIG.load(deps.as_ref().storage).unwrap().mailbox_addr;

let order = FastTransferOrder {
sender: HexBinary::from(left_pad_bytes(
bech32_decode(user_address.as_str()).unwrap(),
32,
)),
recipient: HexBinary::from(left_pad_bytes(
bech32_decode(mailbox_address.as_str()).unwrap(),
32,
)),
amount_in: Uint128::new(100_000_000),
amount_out: Uint128::new(98_000_000),
nonce: 1,
source_domain: 2,
destination_domain: 1,
timeout_timestamp: env.block.time.seconds() + 1000,
data: Some(HexBinary::from(test_payload.clone())),
};

let execute_msg = ExecuteMsg::FillOrder {
filler: Addr::unchecked("solver"),
order: order.clone(),
};

let res = go_fast_transfer_cw::contract::execute(
deps.as_mut(),
env.clone(),
mock_info("solver", &[coin(order.amount_out.u128(), "uusdc")]),
execute_msg.clone(),
)
.unwrap_err()
.to_string();

assert_eq!(res, "Order recipient cannot be mailbox");
}

#[test]
fn test_fill_order_fails_on_incorrect_amount() {
let (mut deps, env) = default_instantiate();
@@ -470,3 +522,44 @@ fn test_fill_order_fails_on_timed_out_order() {

assert_eq!(res, "Order timed out");
}

#[test]
fn test_fill_order_fails_on_timed_out_order_exact() {
let (mut deps, env) = default_instantiate();

let user_address = deps.api.with_prefix("osmo").addr_make("user");

let order = FastTransferOrder {
sender: HexBinary::from(left_pad_bytes(
bech32_decode(user_address.as_str()).unwrap(),
32,
)),
recipient: HexBinary::from(left_pad_bytes(
bech32_decode(user_address.as_str()).unwrap(),
32,
)),
amount_in: Uint128::new(100_000_000),
amount_out: Uint128::new(98_000_000),
nonce: 1,
source_domain: 2,
destination_domain: 1,
timeout_timestamp: env.block.time.seconds(),
data: None,
};

let execute_msg = ExecuteMsg::FillOrder {
filler: Addr::unchecked("solver"),
order: order.clone(),
};

let res = go_fast_transfer_cw::contract::execute(
deps.as_mut(),
env.clone(),
mock_info("solver", &[coin(order.amount_out.u128(), "uusdc")]),
execute_msg.clone(),
)
.unwrap_err()
.to_string();

assert_eq!(res, "Order timed out");
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use common::default_instantiate;
use cosmwasm_std::{testing::mock_info, to_json_binary, Addr, HexBinary, ReplyOn, SubMsg, WasmMsg};
use go_fast::gateway::ExecuteMsg;
use go_fast::{gateway::ExecuteMsg, helpers::keccak256_hash};
use go_fast_transfer_cw::{
helpers::{bech32_decode, left_pad_bytes},
helpers::{bech32_decode, bech32_encode, left_pad_bytes},
state::{self, REMOTE_DOMAINS},
};
use hyperlane::mailbox::{DispatchMsg, ExecuteMsg as MailboxExecuteMsg};
@@ -45,7 +45,12 @@ fn test_initiate_settlement() {
SubMsg {
id: 0,
msg: WasmMsg::Execute {
contract_addr: "mailbox_contract_address".into(),
contract_addr: bech32_encode(
"osmo",
&keccak256_hash("mailbox_contract_address".as_bytes()),
)
.unwrap()
.into_string(),
msg: to_json_binary(&MailboxExecuteMsg::Dispatch(DispatchMsg {
dest_domain: 2,
recipient_addr: HexBinary::from_hex(
@@ -115,7 +120,12 @@ fn test_initiate_settlement_multiple_orders() {
SubMsg {
id: 0,
msg: WasmMsg::Execute {
contract_addr: "mailbox_contract_address".into(),
contract_addr: bech32_encode(
"osmo",
&keccak256_hash("mailbox_contract_address".as_bytes()),
)
.unwrap()
.into_string(),
msg: to_json_binary(&MailboxExecuteMsg::Dispatch(DispatchMsg {
dest_domain: 2,
recipient_addr: HexBinary::from_hex(
@@ -307,3 +317,37 @@ fn test_initiate_settlement_multiple_orders_fails_if_source_domains_are_differen

assert_eq!(res, "Source domains must match");
}

#[test]
fn test_initiate_settlement_fails_if_orders_are_duplicate() {
let (mut deps, env) = default_instantiate();

let solver_address = deps.api.with_prefix("osmo").addr_make("solver");

let order_id = HexBinary::from_hex("1234").unwrap();

state::order_fills()
.create_order_fill(
deps.as_mut().storage,
order_id.clone(),
solver_address.clone(),
2,
)
.unwrap();

let execute_msg = ExecuteMsg::InitiateSettlement {
order_ids: vec![order_id.clone(), order_id.clone()],
repayment_address: HexBinary::from(left_pad_bytes(
bech32_decode(solver_address.as_str()).unwrap(),
32,
)),
};

let info = mock_info(solver_address.as_str(), &[]);

let res = go_fast_transfer_cw::contract::execute(deps.as_mut(), env, info, execute_msg.clone())
.unwrap_err()
.to_string();

assert_eq!(res, "Duplicate order");
}
Loading

0 comments on commit 5c41bd4

Please sign in to comment.