Audit 1 (#154)

* #1mmkbd5 [ACP-02] Removed confusing comment

* #1mmkb8p [ACP-02] Moved check for existing funds when writing to HELD_FUNDS

* #1mmkbe3 [ADD-01] Removed redundant .clone()

* #1mmkbfb [ADD-02] Added unwrap_or_default()

* #1mmkbg3 [ADD-03] Corrected error message for opposing address list modules

* #1mmkbj5 [ADD-04] Removed referenced functions as they are unused.

* #1mmkbnh [ADR-01] Renamed include/exclude address functions

* #1mmkbpx[review] [ADR-02] added comment to help understand includes_address

* #1mmkbnh [ADR-01] Altered exclude_address

* #1mmkbnh [ADR-01] Fixed tests, undid method name changes

* audit_1 [ACK-01] Created timelocked escrow may be immediately unlocked, when provided timestamp in past, fixed with tests

* audit_1 [ACK-01] Created timelocked escrow may be immediately unlocked, when provided timestamp in past, fixed with tests 2

* audit_1 [ACK-02] Functions should have checks to not overwrite existing records in Map.

* audit_1 [ACK-02] Functions should have checks to not overwrite existing records in Map.

* audit_1 [ACK-03] Discussion (through comments) around centralization risk in execute_send() and whether it should be documented.

* 1mmkb8e [ACK-04] splitter may bring denial of service issue

* #1mmkbr2 [AKP-01] Unnecessary type casting

* #1mmkbr2 [AKP-01] Unnecessary type casting

* #1mmkbrr [AKP-02] Possible integer overflow

* #1mmkby3 [COM-03] Ensure that 0 <= fee_rate <= 100 also rate in Taxable.

* #1mmkbyf [COM-4] Ensure that Coin::amount <= Uint128::MAX/100

* #1mmkce9 [REC-01] Consider replacing ok_or with lazily evaluated ok_or_else

* #1mmkcg8 [TAX-01] Ensure that 0 <= fee_rate <= 100 also rate in taxable

* #1mmkb8e [ACK-04] splitter may bring denial of service issue

* #1mmkcew [ROY-01] on_agreed_transfer() Hook not consistent between royalties.rs and taxable.rs

* #1mmkbxm [COM-02] Proper usage of &Vec<> over &[]

* #1mmkcjf [TOK-02] Name of add_approval() is misleading and not documented

* #1mmkawq [GLOBAL-02] Using if let or match instead of is_some+unwrap()

* #1mmkche [TOK-01] Missing sanity validation in InstantiateMsg::Validate()

* #1mmkc5w [COT-01] Added check for token_id uniqueness

* Linting

* 1mmkc8w [COT-05] Altered Token in state to be optional to allow archiving of burnt token ids

* Removed .is_some() from token state functions

* Renamed save token function to better suit functionality

* added [TOK-01] blacklists + test

* Fixed tests issues caused by changing token state to optional

* #1mmkbwc [COM-01] Possible integer overflow

* #1mmkc0u [COM-05] misleading error message in deduct_payment()

* #1mmkccf [MOD-01] Cloning used for types that implement the Copy trait

* #1mmkcfr [STA-01] Lacking document of functionality of increment_num_tokens()

* #1mmkbtq [CKP-01] Altered type casting for u128

* #1mmkc55 [CON-01] Removed unnecessary .clone() calls

* #1mmkcge [TAX-02] Added comment related to payments for tax/royalty

* #1mmkaw7 [GLOBAL-01] Removed various unnecessary uses of .clone()

* Tagged variables as unused in tests

* #1mmkb3j [GLOBAL-03] Added comments to public package functions

* Fixed failing test

* added FETCH smart contract

* [COM-02] Added a check for the payment amount to ensure it cannot exceed the limitations of Uin128

Co-authored-by: unknown <georgeschouchani1@gmail.com>
Co-authored-by: Georges Chouchani <gachouchani@ndu.edu.lb>

contracts/andromeda_addresslist/src/contract.rs

@@ -1,7 +1,7 @@
 use andromeda_protocol::{
     address_list::{AddressList, ExecuteMsg, IncludesAddressResponse, InstantiateMsg, QueryMsg},
     ownership::{execute_update_owner, query_contract_owner, CONTRACT_OWNER},
-    require::require,
+    require,
 };
 use cosmwasm_std::{
     attr, entry_point, to_binary, Binary, Deps, DepsMut, Env, MessageInfo, Response, StdError,
@@ -23,7 +23,7 @@ pub fn instantiate(
     let state = State {
         owner: info.sender.to_string(),
         address_list: AddressList {
-            moderators: msg.moderators.clone(),
+            moderators: msg.moderators,
         },
     };
 
@@ -82,10 +82,7 @@ fn execute_remove_address(
         StdError::generic_err("Only a moderator can remove an address from the address list"),
     )?;
 
-    state
-        .address_list
-        .remove_address(deps.storage, &address)
-        .unwrap();
+    state.address_list.remove_address(deps.storage, &address);
     STATE.save(deps.storage, &state)?;
 
     Ok(Response::new().add_attributes(vec![
@@ -124,7 +121,7 @@ mod tests {
         let msg = InstantiateMsg {
             moderators: vec!["11".to_string(), "22".to_string()],
         };
-        let res = instantiate(deps.as_mut(), env, info.clone(), msg).unwrap();
+        let res = instantiate(deps.as_mut(), env, info, msg).unwrap();
         assert_eq!(0, res.messages.len());
     }
 
@@ -155,7 +152,7 @@ mod tests {
 
         //add address for registered moderator
 
-        let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()).unwrap();
+        let res = execute(deps.as_mut(), env.clone(), info, msg.clone()).unwrap();
         let expected = Response::default().add_attributes(vec![
             attr("action", "add_address"),
             attr("address", address),
@@ -182,8 +179,7 @@ mod tests {
 
         //add address for unregistered moderator
         let unauth_info = mock_info("anyone", &[]);
-        let res =
-            execute(deps.as_mut(), env.clone(), unauth_info.clone(), msg.clone()).unwrap_err();
+        let res = execute(deps.as_mut(), env, unauth_info, msg).unwrap_err();
         assert_eq!(
             StdError::generic_err("Only a moderator can add an address to the address list"),
             res
@@ -216,22 +212,21 @@ mod tests {
         };
 
         //add address for registered moderator
-        let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()).unwrap();
+        let res = execute(deps.as_mut(), env.clone(), info, msg.clone()).unwrap();
         let expected = Response::default().add_attributes(vec![
             attr("action", "remove_address"),
             attr("address", address.to_string()),
         ]);
         assert_eq!(expected, res);
 
-        let included = ADDRESS_LIST
+        let included_is_err = ADDRESS_LIST
             .load(deps.as_ref().storage, address.to_string())
-            .unwrap();
-        assert_eq!(false, included);
+            .is_err();
+        assert_eq!(true, included_is_err);
 
         //add address for unregistered moderator
         let unauth_info = mock_info("anyone", &[]);
-        let res =
-            execute(deps.as_mut(), env.clone(), unauth_info.clone(), msg.clone()).unwrap_err();
+        let res = execute(deps.as_mut(), env, unauth_info.clone(), msg).unwrap_err();
         assert_eq!(
             StdError::generic_err("Only a moderator can remove an address from the address list"),
             res

contracts/andromeda_factory/src/contract.rs

@@ -2,7 +2,7 @@ use andromeda_protocol::{
     factory::{AddressResponse, CodeIdsResponse, ExecuteMsg, InstantiateMsg, QueryMsg},
     modules::ModuleDefinition,
     ownership::{execute_update_owner, is_contract_owner, query_contract_owner, CONTRACT_OWNER},
-    require::require,
+    require,
     token::InstantiateMsg as TokenInstantiateMsg,
 };
 use cosmwasm_std::{
@@ -106,7 +106,7 @@ pub fn create(
             } => ModuleDefinition::Whitelist {
                 address: address.clone(),
                 moderators: moderators.clone(),
-                code_id: Some(config.address_list_code_id.clone()),
+                code_id: Some(config.address_list_code_id),
             },
             ModuleDefinition::Blacklist {
                 address,
@@ -115,7 +115,7 @@ pub fn create(
             } => ModuleDefinition::Blacklist {
                 address: address.clone(),
                 moderators: moderators.clone(),
-                code_id: Some(config.address_list_code_id.clone()),
+                code_id: Some(config.address_list_code_id),
             },
             ModuleDefinition::Receipt {
                 address,
@@ -124,7 +124,7 @@ pub fn create(
             } => ModuleDefinition::Receipt {
                 address: address.clone(),
                 moderators: moderators.clone(),
-                code_id: Some(config.receipt_code_id.clone()),
+                code_id: Some(config.receipt_code_id),
             },
             _ => m.clone(),
         })
@@ -136,6 +136,13 @@ pub fn create(
         minter: info.sender.to_string(),
         modules: updated_modules,
     };
+    // [TOK-01 Validation Process]
+    let validation = token_inst_msg.validate();
+    match validation {
+        Ok(true) => {}
+        Err(error) => panic!("{:?}", error),
+        _ => {}
+    };
 
     let inst_msg = WasmMsg::Instantiate {
         admin: Some(info.sender.to_string()),
@@ -172,7 +179,7 @@ pub fn update_address(
         StdError::generic_err("Cannot update address for ADO that you did not create"),
     )?;
 
-    store_address(deps.storage, symbol.clone(), &new_address.clone())?;
+    store_address(deps.storage, symbol, &new_address)?;
 
     Ok(Response::default())
 }
@@ -192,16 +199,19 @@ pub fn update_code_id(
     )?;
     let mut config = read_config(deps.storage)?;
 
-    if receipt_code_id.is_some() {
-        config.receipt_code_id = receipt_code_id.unwrap();
+    // [GLOBAL-02] Changing is_some() + .unwrap() to if let Some()
+    if let Some(receipt_code_id) = receipt_code_id {
+        config.receipt_code_id = receipt_code_id;
     }
 
-    if address_list_code_id.is_some() {
-        config.address_list_code_id = address_list_code_id.unwrap();
+    // [GLOBAL-02] Changing is_some() + .unwrap() to if let Some()
+    if let Some(address_list_code_id) = address_list_code_id {
+        config.address_list_code_id = address_list_code_id;
     }
 
-    if token_code_id.is_some() {
-        config.token_code_id = token_code_id.unwrap();
+    // [GLOBAL-02] Changing is_some() + .unwrap() to if let Some()
+    if let Some(token_code_id) = token_code_id {
+        config.token_code_id = token_code_id;
     }
 
     store_config(deps.storage, &config)?;
@@ -228,9 +238,7 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult<Binary> {
 
 fn query_address(deps: Deps, symbol: String) -> StdResult<AddressResponse> {
     let address = read_address(deps.storage, symbol)?;
-    Ok(AddressResponse {
-        address: address.clone(),
-    })
+    Ok(AddressResponse { address })
 }
 
 fn query_code_ids(deps: Deps) -> StdResult<CodeIdsResponse> {
@@ -259,7 +267,7 @@ mod tests {
 
     static ADDRESS_LIST_CODE_ID: u64 = 2;
     const TOKEN_NAME: &str = "test";
-    const TOKEN_SYMBOL: &str = "T";
+    const TOKEN_SYMBOL: &str = "TT";
 
     #[test]
     fn proper_initialization() {
@@ -303,6 +311,13 @@ mod tests {
             minter: info.sender.to_string(),
             modules: vec![],
         };
+        // [TOK-01 Validation Process]
+        let validation = token_inst_msg.validate();
+        match validation {
+            Ok(true) => {}
+            Err(error) => panic!("{:?}", error),
+            _ => {}
+        };
 
         let inst_msg = WasmMsg::Instantiate {
             admin: Some(info.sender.to_string()),
@@ -338,11 +353,9 @@ mod tests {
         let owner = String::from("owner");
         let mut deps = mock_dependencies_custom(&[]);
         let env = mock_env();
-        let info = mock_info(creator.clone().as_str(), &[]);
+        let info = mock_info(creator.as_str(), &[]);
 
-        CONTRACT_OWNER
-            .save(deps.as_mut().storage, &owner.clone())
-            .unwrap();
+        CONTRACT_OWNER.save(deps.as_mut().storage, &owner).unwrap();
         SYM_ADDRESS
             .save(
                 deps.as_mut().storage,
@@ -359,32 +372,26 @@ mod tests {
 
         let unauth_env = mock_env();
         let unauth_info = mock_info("anyone", &[]);
-        let unauth_res = execute(
-            deps.as_mut(),
-            unauth_env.clone(),
-            unauth_info.clone(),
-            update_msg.clone(),
-        )
-        .unwrap_err();
+        let unauth_res =
+            execute(deps.as_mut(), unauth_env, unauth_info, update_msg.clone()).unwrap_err();
 
         assert_eq!(
             unauth_res,
             StdError::generic_err("Cannot update address for ADO that you did not create"),
         );
 
-        let update_res =
-            execute(deps.as_mut(), env.clone(), info.clone(), update_msg.clone()).unwrap();
+        let update_res = execute(deps.as_mut(), env.clone(), info, update_msg).unwrap();
 
         assert_eq!(update_res, Response::default());
 
         let query_msg = QueryMsg::GetAddress {
             symbol: TOKEN_SYMBOL.to_string(),
         };
 
-        let addr_res = query(deps.as_ref(), env.clone(), query_msg).unwrap();
+        let addr_res = query(deps.as_ref(), env, query_msg).unwrap();
         let addr_val: AddressResponse = from_binary(&addr_res).unwrap();
 
-        assert_eq!(new_address.clone(), addr_val.address);
+        assert_eq!(new_address, addr_val.address);
     }
 
     #[test]
@@ -401,23 +408,15 @@ mod tests {
         };
         store_config(deps.as_mut().storage, &config).unwrap();
 
-        CONTRACT_OWNER
-            .save(deps.as_mut().storage, &owner.clone())
-            .unwrap();
+        CONTRACT_OWNER.save(deps.as_mut().storage, &owner).unwrap();
 
         let invalid_msg = ExecuteMsg::UpdateCodeId {
             receipt_code_id: None,
             token_code_id: None,
             address_list_code_id: None,
         };
 
-        let resp = execute(
-            deps.as_mut(),
-            env.clone(),
-            info.clone(),
-            invalid_msg.clone(),
-        )
-        .unwrap_err();
+        let resp = execute(deps.as_mut(), env.clone(), info.clone(), invalid_msg).unwrap_err();
         let expected = StdError::generic_err("Must provide one of the following: \"receipt_code_id\", \"token_code_id\", \"address_list_code_id\"");
 
         assert_eq!(resp, expected);
@@ -429,13 +428,12 @@ mod tests {
             address_list_code_id: None,
         };
 
-        let resp =
-            execute(deps.as_mut(), env.clone(), unauth_info.clone(), msg.clone()).unwrap_err();
+        let resp = execute(deps.as_mut(), env.clone(), unauth_info, msg.clone()).unwrap_err();
         let expected = StdError::generic_err("Can only be used by the contract owner");
 
         assert_eq!(resp, expected);
 
-        let resp = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()).unwrap();
+        let resp = execute(deps.as_mut(), env, info, msg).unwrap();
         let expected = Response::default().add_attributes(vec![
             attr("action", "update_code_id"),
             attr("receipt_code_id", new_receipt_code_id.to_string()),

contracts/andromeda_factory/src/testing/tests.rs

@@ -15,7 +15,7 @@ use cosmwasm_std::{
 
 static TOKEN_CODE_ID: u64 = 0;
 const TOKEN_NAME: &str = "test";
-const TOKEN_SYMBOL: &str = "T";
+const TOKEN_SYMBOL: &str = "TT";
 const ADDRESS_LIST_CODE_ID: u64 = 1;
 const RECEIPT_CODE_ID: u64 = 2;
 
@@ -82,7 +82,7 @@ fn test_create() {
         name: TOKEN_NAME.to_string(),
         symbol: TOKEN_SYMBOL.to_string(),
         minter: info.sender.to_string(),
-        modules: modules.clone(),
+        modules,
     };
 
     let inst_msg = WasmMsg::Instantiate {
@@ -108,7 +108,7 @@ fn test_create() {
             attr("symbol", TOKEN_SYMBOL.to_string()),
         ]);
 
-    let res = execute(deps.as_mut(), mock_env(), info.clone(), msg).unwrap();
+    let res = execute(deps.as_mut(), env, info, msg).unwrap();
     assert_eq!(res, expected_res);
     assert_eq!(1, expected_res.messages.len())
 }

contracts/andromeda_receipt/src/contract.rs

@@ -7,7 +7,7 @@ use andromeda_protocol::{
         Config, ContractInfoResponse, ExecuteMsg, InstantiateMsg, QueryMsg, Receipt,
         ReceiptResponse,
     },
-    require::require,
+    require,
 };
 use cosmwasm_std::{
     attr, entry_point, to_binary, Binary, Deps, DepsMut, Env, MessageInfo, Response, StdError,
@@ -132,7 +132,7 @@ mod tests {
             minter: owner.to_string(),
             moderators: None,
         };
-        let res = instantiate(deps.as_mut(), env, info.clone(), msg).unwrap();
+        let res = instantiate(deps.as_mut(), env, info, msg).unwrap();
         assert_eq!(0, res.messages.len());
     }
 
@@ -166,7 +166,7 @@ mod tests {
         );
 
         //add address for registered moderator
-        let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()).unwrap();
+        let res = execute(deps.as_mut(), env, info, msg).unwrap();
         assert_eq!(
             Response::new().add_attributes(vec![
                 attr("action", "mint_receipt"),
@@ -212,20 +212,19 @@ mod tests {
             events: vec![Event::new("new")],
         };
         let msg = ExecuteMsg::EditReceipt {
-            receipt_id: Uint128::from(1 as u128),
+            receipt_id: Uint128::from(1_u128),
             receipt: new_receipt.clone(),
         };
 
-        let res_unauth =
-            execute(deps.as_mut(), env.clone(), unauth_info.clone(), msg.clone()).unwrap_err();
+        let res_unauth = execute(deps.as_mut(), env.clone(), unauth_info, msg.clone()).unwrap_err();
         assert_eq!(
             res_unauth,
             StdError::generic_err(
                 "Only the contract owner, the assigned minter or a moderator can edit a receipt"
             )
         );
 
-        let res = execute(deps.as_mut(), env.clone(), info.clone(), msg.clone()).unwrap();
+        let res = execute(deps.as_mut(), env.clone(), info.clone(), msg).unwrap();
         let expected = Response::default().add_attributes(vec![
             attr("action", "edit_receipt"),
             attr("receipt_id", "1"),
@@ -235,9 +234,9 @@ mod tests {
         assert_eq!(res, expected);
 
         let query_msg = QueryMsg::Receipt {
-            receipt_id: Uint128::from(1 as u128),
+            receipt_id: Uint128::from(1_u128),
         };
-        let res = query(deps.as_ref(), env.clone(), query_msg).unwrap();
+        let res = query(deps.as_ref(), env, query_msg).unwrap();
         let val: ReceiptResponse = from_binary(&res).unwrap();
 
         assert_eq!(val.receipt, new_receipt)