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

Audit final revision compared to initial audit (based on branch is v0.1.5, commit f1dfefc71c3ace567a5b79e98100ee17d9cfcc5d) #88

Closed
Closed
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
04fd6a8
new CW721_ADMIN store
taitruong Jan 21, 2024
5016c3a
use latest optimizer version
taitruong Jan 21, 2024
b5c4acb
rename CW721_ADMIN > ADMIN_USED_FOR_CW721
taitruong Jan 22, 2024
09da354
Merge pull request #81 from arkprotocol/cw721_admin
taitruong Jan 23, 2024
fb52da8
test backtransfer to banned recipient, but also to to regular recpient
taitruong Jan 25, 2024
be897b8
e2e ts relayer test:
taitruong Jan 26, 2024
ae6c7d3
fix back transfer, remove entries in outgoing channel only in case al…
taitruong Jan 26, 2024
2afbce7
rename
taitruong Jan 26, 2024
6357673
simplify receive_ibc_packet(), create sub message directly, remove ac…
taitruong Jan 27, 2024
f903de4
fix move to sub message for saving incoming channel entries
taitruong Jan 27, 2024
f43f9c0
move to dedicated functions
taitruong Jan 27, 2024
31ab7b5
docs
taitruong Jan 27, 2024
dec10c1
cleanup
taitruong Jan 27, 2024
94557a5
cargo schema
taitruong Jan 28, 2024
c5a6548
2 new admin msgs for fixing forked NFTs
taitruong Jan 29, 2024
80995ed
docs
taitruong Jan 29, 2024
dc32c5f
test admin msgs
taitruong Jan 29, 2024
545909f
cargo schema
taitruong Jan 29, 2024
1d96023
Merge pull request #83 from arkprotocol/fix_back_transfer
taitruong Jan 31, 2024
2da4752
rename
taitruong Feb 1, 2024
5059fc0
pass nft contract, instead of overriding info.sender
taitruong Jan 22, 2024
d6d6383
remove NFT_CONTRACT_TO_CLASS_ID and CLASS_ID_TO_NFT_CONTRACT and merg…
taitruong Jan 23, 2024
36e0567
test migration
taitruong Jan 23, 2024
86537cb
migrate only in case CLASS_ID_AND_NFT_CONTRACT_INFO is not populated …
taitruong Feb 1, 2024
f7b1b90
fix optional minter
taitruong Feb 9, 2024
7ecfb36
typo
taitruong Feb 12, 2024
2d19b4d
use Stargaze icon as placeholder
taitruong Feb 14, 2024
e8e3c1f
move to constant
taitruong Feb 15, 2024
bbc19e8
move to dedicated function
taitruong Feb 15, 2024
9ef9bd7
docs
taitruong Feb 16, 2024
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
Prev Previous commit
Next Next commit
2 new admin msgs for fixing forked NFTs
taitruong committed Jan 29, 2024
commit c5a6548ae2364d547189cf3334e34b45494b83da
15 changes: 15 additions & 0 deletions packages/ics721/src/error.rs
Original file line number Diff line number Diff line change
@@ -27,6 +27,13 @@ pub enum ContractError {
#[error("NFT not escrowed by ICS721! Owner: {0}")]
NotEscrowedByIcs721(String),

#[error("{recipient} not owner of NFT {token_id}! Owner: {owner}")]
NotOwnerOfNft {
recipient: String,
owner: String,
token_id: String,
},

#[error("only unordered channels are supported")]
OrderedChannel {},

@@ -50,4 +57,12 @@ pub enum ContractError {

#[error("Couldn't find nft contract for this class id: {0}")]
NoNftContractForClassId(String),

#[error("Unknown nft contract: {child_collection}, Class Id: {class_id}, Token ID: {token_id} => NFT contract: {cw721_addr}")]
NoClassIdForNftContract {
child_collection: String,
class_id: String,
token_id: String,
cw721_addr: String,
},
}
163 changes: 163 additions & 0 deletions packages/ics721/src/execute.rs
Original file line number Diff line number Diff line change
@@ -98,7 +98,170 @@ where
}) => self.execute_receive_nft(deps, env, info, token_id, sender, msg),
ExecuteMsg::Pause {} => self.execute_pause(deps, info),
ExecuteMsg::Callback(msg) => self.execute_callback(deps, env, info, msg),
ExecuteMsg::AdminCleanAndBurnNft {
recipient,
token_id,
class_id,
collection,
} => self.execute_admin_clean_and_burn_nft(
deps, env, info, recipient, token_id, class_id, collection,
),
ExecuteMsg::AdminCleanAndUnescrowNft {
recipient,
token_id,
class_id,
collection,
} => self.execute_admin_clean_and_unescrow_nft(
deps, env, info, recipient, token_id, class_id, collection,
),
}
}

#[allow(clippy::too_many_arguments)]
fn execute_admin_clean_and_burn_nft(
&self,
deps: DepsMut,
env: Env,
info: MessageInfo,
recipient: String,
token_id: String,
child_class_id: String,
child_collection: String,
) -> Result<Response<T>, ContractError> {
deps.api.addr_validate(&recipient)?;
// only admin can call this method
let ContractInfoResponse { admin, .. } = deps
.querier
.query_wasm_contract_info(env.contract.address.to_string())?;
if admin.is_some() && info.sender != admin.unwrap() {
return Err(ContractError::Unauthorized {});
}

// check given child class id and child collection is the same as stored in the contract
let token_id = TokenId::new(token_id);
let child_class_id = ClassId::new(child_class_id);
let child_collection = deps.api.addr_validate(&child_collection)?;
let cw721_addr = CLASS_ID_TO_NFT_CONTRACT.load(deps.storage, child_class_id.clone())?;
if cw721_addr != child_collection {
return Err(ContractError::NoClassIdForNftContract {
child_collection: child_collection.to_string(),
class_id: child_class_id.to_string(),
token_id: token_id.into(),
cw721_addr: cw721_addr.to_string(),
});
}

// remove incoming channel entry and metadata
INCOMING_CLASS_TOKEN_TO_CHANNEL
.remove(deps.storage, (child_class_id.clone(), token_id.clone()));
TOKEN_METADATA.remove(deps.storage, (child_class_id.clone(), token_id.clone()));

// check NFT on child collection owned by recipient
let maybe_nft_info: Option<UniversalAllNftInfoResponse> = deps
.querier
.query_wasm_smart(
child_collection.clone(),
&cw721::Cw721QueryMsg::AllNftInfo {
token_id: token_id.clone().into(),
include_expired: None,
},
)
.ok();

let mut response =
Response::default().add_attribute("method", "execute_admin_clean_and_burn_nft");
if let Some(UniversalAllNftInfoResponse { access, .. }) = maybe_nft_info {
if access.owner != recipient {
return Err(ContractError::NotOwnerOfNft {
recipient: recipient.to_string(),
token_id: token_id.clone().into(),
owner: access.owner.to_string(),
});
}
// burn child NFT
let burn_msg = WasmMsg::Execute {
contract_addr: child_collection.to_string(),
msg: to_json_binary(&cw721::Cw721ExecuteMsg::Burn {
token_id: token_id.clone().into(),
})?,
funds: vec![],
};
response = response.add_message(burn_msg);
}

Ok(response)
}

#[allow(clippy::too_many_arguments)]
fn execute_admin_clean_and_unescrow_nft(
&self,
deps: DepsMut,
env: Env,
info: MessageInfo,
recipient: String,
token_id: String,
home_class_id: String,
home_collection: String,
) -> Result<Response<T>, ContractError> {
deps.api.addr_validate(&recipient)?;
// only admin can call this method
let ContractInfoResponse { admin, .. } = deps
.querier
.query_wasm_contract_info(env.contract.address.to_string())?;
if admin.is_some() && info.sender != admin.unwrap() {
return Err(ContractError::Unauthorized {});
}

// check given home class id and home collection is the same as stored in the contract
let home_class_id = ClassId::new(home_class_id);
let home_collection = deps.api.addr_validate(&home_collection)?;
let cw721_addr = CLASS_ID_TO_NFT_CONTRACT.load(deps.storage, home_class_id.clone())?;
if cw721_addr != home_collection {
return Err(ContractError::NoClassIdForNftContract {
child_collection: home_collection.to_string(),
class_id: home_class_id.to_string(),
token_id,
cw721_addr: cw721_addr.to_string(),
});
}

// remove outgoing channel entry
let token_id = TokenId::new(token_id);
OUTGOING_CLASS_TOKEN_TO_CHANNEL
.remove(deps.storage, (home_class_id.clone(), token_id.clone()));

// check NFT on home collection owned by ics721 contract
let maybe_nft_info: Option<UniversalAllNftInfoResponse> = deps
.querier
.query_wasm_smart(
home_collection.clone(),
&cw721::Cw721QueryMsg::AllNftInfo {
token_id: token_id.clone().into(),
include_expired: None,
},
)
.ok();

let mut response =
Response::default().add_attribute("method", "execute_admin_clean_and_unescrow_nft");
if let Some(UniversalAllNftInfoResponse { access, .. }) = maybe_nft_info {
if access.owner != env.contract.address {
return Err(ContractError::NotEscrowedByIcs721(access.owner.to_string()));
}
// transfer NFT
let transfer_msg = WasmMsg::Execute {
contract_addr: home_collection.to_string(),
msg: to_json_binary(&cw721::Cw721ExecuteMsg::TransferNft {
recipient: recipient.to_string(),
token_id: token_id.clone().into(),
})?,
funds: vec![],
};

response = response.add_message(transfer_msg);
}

Ok(response)
}

/// ICS721 may receive an NFT from 2 sources:
19 changes: 19 additions & 0 deletions packages/ics721/src/msg.rs
Original file line number Diff line number Diff line change
@@ -46,6 +46,25 @@ pub enum ExecuteMsg {
/// Mesages used internally by the contract. These may only be
/// called by the contract itself.
Callback(CallbackMsg),

/// Admin msg in case something goes wrong.
/// As a minimum it clean up states (incoming channel and token metadata), and burn NFT if exists.
AdminCleanAndBurnNft {
recipient: String,
token_id: String,
class_id: String,
collection: String,
},

/// Admin msg in case something goes wrong.
/// As a minimum it clean up state (outgoing channel), and transfer NFT if exists.
/// - transfer NFT if exists
AdminCleanAndUnescrowNft {
recipient: String,
token_id: String,
class_id: String,
collection: String,
},
}

#[cw_serde]
161 changes: 158 additions & 3 deletions packages/ics721/src/testing/integration_tests.rs
Original file line number Diff line number Diff line change
@@ -21,7 +21,7 @@ use crate::{
ibc::Ics721Ibc,
msg::{CallbackMsg, ExecuteMsg, InstantiateMsg, MigrateMsg, QueryMsg},
query::Ics721Query,
state::CollectionData,
state::{CollectionData, UniversalAllNftInfoResponse},
token_types::VoucherCreation,
ContractError,
};
@@ -353,11 +353,11 @@ impl Test {
incoming_proxy,
outgoing_proxy,
pauser: admin.clone(),
cw721_admin: admin,
cw721_admin: admin.clone(),
},
&[],
"ics721-base",
admin_and_pauser.map(|p| app.api().addr_make(&p).to_string()),
admin.clone(),
)
.unwrap();

@@ -510,6 +510,19 @@ impl Test {
.unwrap()
}

fn query_cw721_all_nft_info(&mut self, token_id: String) -> UniversalAllNftInfoResponse {
self.app
.wrap()
.query_wasm_smart(
self.source_cw721.clone(),
&cw721_base::msg::QueryMsg::<Empty>::AllNftInfo {
token_id,
include_expired: None,
},
)
.unwrap()
}

fn execute_cw721_mint(&mut self, owner: Addr) -> Result<String, anyhow::Error> {
self.nfts_minted += 1;

@@ -1992,6 +2005,148 @@ fn test_receive_nft() {
}
}

#[test]
fn test_admin_clean_and_unescrow_nft() {
// test case: receive nft from cw721-base
{
let mut test = Test::new(
false,
false,
None,
Some(ICS721_ADMIN_AND_PAUSER.to_string()),
cw721_base_contract(),
true,
);
// simplify: mint and escrowed/owned by ics721, as a precondition for receive nft
let token_id_escrowed_by_ics721 = test.execute_cw721_mint(test.ics721.clone()).unwrap();
let recipient = test.app.api().addr_make("recipient");
let token_id_from_owner = test.execute_cw721_mint(recipient.clone()).unwrap();
let channel = "channel-0".to_string();
test.app
.execute_contract(
test.source_cw721.clone(),
test.ics721.clone(),
&ExecuteMsg::ReceiveNft(cw721::Cw721ReceiveMsg {
sender: test.source_cw721_owner.to_string(),
token_id: token_id_escrowed_by_ics721.clone(),
msg: to_json_binary(&IbcOutgoingMsg {
receiver: NFT_OWNER_TARGET_CHAIN.to_string(), // nft owner for other chain, on this chain ics721 is owner
channel_id: channel.clone(),
timeout: IbcTimeout::with_block(IbcTimeoutBlock {
revision: 0,
height: 10,
}),
memo: None,
})
.unwrap(),
}),
&[],
)
.unwrap();
// check outgoing channel entry
let outgoing_channel = test.query_outgoing_channels();
assert_eq!(outgoing_channel.len(), 1);
let class_id = ClassId::new(test.source_cw721.to_string());
assert_eq!(
outgoing_channel,
vec![(
(class_id.to_string(), token_id_escrowed_by_ics721.clone()),
channel.clone()
)]
);
// assert nft is escrowed
let UniversalAllNftInfoResponse { access, .. } =
test.query_cw721_all_nft_info(token_id_escrowed_by_ics721.clone());
assert_eq!(access.owner, test.ics721.to_string());

// non admin can't call
let non_admin = test.app.api().addr_make("not_admin");
let admin = test.app.api().addr_make(ICS721_ADMIN_AND_PAUSER);
let clean_and_burn_msg = ExecuteMsg::AdminCleanAndBurnNft {
recipient: recipient.to_string(),
token_id: token_id_escrowed_by_ics721.clone(),
class_id: class_id.to_string(),
collection: test.source_cw721.to_string(),
};
let err: ContractError = test
.app
.execute_contract(
non_admin.clone(),
test.ics721.clone(),
&clean_and_burn_msg,
&[],
)
.unwrap_err()
.downcast()
.unwrap();
assert_eq!(err, ContractError::Unauthorized {});

let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft {
recipient: recipient.to_string(),
token_id: token_id_from_owner.clone(), // not escrowed by ics721
class_id: class_id.to_string(),
collection: test.source_cw721.to_string(),
};
let err: ContractError = test
.app
.execute_contract(
admin.clone(),
test.ics721.clone(),
&clean_and_unescrow_msg,
&[],
)
.unwrap_err()
.downcast()
.unwrap();
assert_eq!(
err,
ContractError::NotEscrowedByIcs721(recipient.to_string())
);

// unknown class id
let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft {
recipient: recipient.to_string(),
token_id: token_id_escrowed_by_ics721.to_string(),
class_id: "unknown".to_string(),
collection: test.source_cw721.to_string(),
};
let err: ContractError = test
.app
.execute_contract(
admin.clone(),
test.ics721.clone(),
&clean_and_unescrow_msg,
&[],
)
.unwrap_err()
.downcast()
.unwrap();
assert_eq!(err, ContractError::Std(StdError::NotFound { kind: "type: cosmwasm_std::addresses::Addr; key: [00, 01, 65, 75, 6E, 6B, 6E, 6F, 77, 6E]".to_string() }));

let clean_and_unescrow_msg = ExecuteMsg::AdminCleanAndUnescrowNft {
recipient: recipient.to_string(),
token_id: token_id_escrowed_by_ics721.clone(),
class_id: class_id.to_string(),
collection: test.source_cw721.to_string(),
};
test.app
.execute_contract(
admin.clone(),
test.ics721.clone(),
&clean_and_unescrow_msg,
&[],
)
.unwrap();
// asert outgoing channel entry is removed
let outgoing_channel = test.query_outgoing_channels();
assert_eq!(outgoing_channel.len(), 0);
// check nft is unescrowed
let UniversalAllNftInfoResponse { access, .. } =
test.query_cw721_all_nft_info(token_id_escrowed_by_ics721.clone());
assert_eq!(access.owner, recipient.to_string());
}
}

/// In case proxy for ICS721 is defined, ICS721 only accepts receival from proxy - not from nft contract!
#[test]
fn test_no_receive_with_proxy() {

Unchanged files with check annotations Beta

const osmosis = { ...oldOsmo, minFee: "0.025uosmo" };
export function bigIntReplacer(_key: string, value: any) {

Check warning on line 29 in ts-relayer-tests/src/utils.ts

GitHub Actions / build (18.x)

Unexpected any. Specify a different type

Check warning on line 29 in ts-relayer-tests/src/utils.ts

GitHub Actions / build (18.x)

Unexpected any. Specify a different type
if (typeof value === "bigint") {
return value.toString();
}