Orderly Network

CertiK16 Sep 2022Code version: not recorded (see the report)

CertiK reviewed Orderly Network on 16 Sep 2022: 28 findings, 0 critical; 26 marked fixed, 2 open.

No open critical findings

Open the report Open Orderly Network’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings28

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	1	1
Medium	0	8
Low	1	17

HighPotential Million Small Deposits AttackFixed

There does not exist a storage management system for determining if the contract holds sufficient `near` to maintain storage. Further, user's are not responsible for storage fees. As a result, the `Asset Manager` contract is responsible for paying for storage. However this leaves the contract vulnerable to the `Million Small Deposits Attack`. This can lead the contract to being cost prohibitive to call. For example, the `create_user_account()` is a public function. The more users that call this function, the larger the stake balance is needed to cover storage. Another function call that needs proper storage fees is `operator_execute_action()`. Users are not responsible for sending trades to chain and updating the balances. An incredible amount of small volume trades, could possibly create unintended results without proper management. There are other functions that also write to storage but are less vulnerable to such an attack. For example in owner.rs, the following functions update the state of the contract. These functions should require Near to call and should track the deposited stake: 1. The function `create_approver_request` updates the state of the LookupMap `approve_requests`. 2. The function `add_authorized_account` updates the state of the LookupSet `authorized_users`.

Medium`new_account_approved` Approval LogicFixed

The function `new_account_approved`, updates an approval request dependent on the number of approvals. Further, an approval request will only be successfully approved if the new role account and a sufficient number of approvers agree to the request. However consider the following case where the approval request will not be completed. Assume the following: 1. There are 10 approvers 2. The number of approvals required is 6 = 10/2+1 3. The "SetOwner" request is in progress. There is no maximum limit on the number of approvers allowed to approve a request. Therefore the `approved_by' field can be larger then the sufficient amount. If the `approved_by` is strictly larger then the sufficient amount then the `request.approved_by.is_set_enough()` function will return false. The new "owner" account will never be successfully approved. And another new `SetOwner` request cannot be created because of that stuck request.

MediumFunction `user_request_withdraw()` Not Marked PayFixed

The `user_request_withdraw()` function requires users to deposit 1 yoctoNear to withdraw assets, however, the function itself is not marked as `payable`.

MediumInaccurate Storage Usage CalculationFixed

Near provides two methods either by manual byte math or using the SDK environment. The manual method is difficult and requires manual gas measurements. On the other hand, checking [`env.storage_usage()`](https://docs.near.org/concepts/storage/storage-staking#calculate-costs-for-your-own-contract) is a more accessible method. In the codebase, a user's storage cost is calculated based on the manual byte math. We provide the user registration example to show that the method used in the codebase contains an inaccuracy. When registering a new user in the `asset manager` contract, the types `AccountId` and `AccountV2` data will be saved on the chain. The const variable `INIT_ACCOUNT_WITH_ONE_KEY_STORAGE`, whose value is 358 bytes, denotes the initial size of the above two data types and also the minimum attached deposit for registering a new user. Actually, the storage usage denoted by `INIT_ACCOUNT_WITH_ONE_KEY_STORAGE` is inaccurate. The following test code's output shows that the value of `INIT_ACCOUNT_WITH_ONE_KEY_STORAGE` is smaller than the real storage usage for registering a user. In this case, the contract itself needs to pay for a part of the users' storage usage. [`Test Code`]: ```rust // As needed, we change the scope of the `AccountV2`'s function `from_user` and field `keys` from `public(crate)` to `public` use asset_manager::{account_deposit::*, types::*}; fn main() { let initial_storage = env::storage_usage() as u128; let alice = AccountId::try_from("alice.near".to_string()).unwrap(); let mut user_record = AccountV2::from_user(&alice); let orderly_key: PublicKey = "ed25519:6E8sCci9badyRkXb3JoRpBj5p8C6Tw41ELDZoiihKEtp".parse().unwrap(); user_record.keys.insert(&orderly_key, &TradingKey::default()); let mut user_ledger: UnorderedMap<AccountId, VAccount> = UnorderedMap::new(StorageKey::CurrentUsers); user_ledger.insert(&alice, &VAccount::V2(user_record)); let current_storage = env::storage_usage() as u128; let storage_diff = current_storage - initial_storage; println!("Calculating new user storage cost by calling SDK:"); println!("initial storage: {}, current storage: {}, storage cost: {}", initial_storage, current_storage, storage_diff); println!("Calculating new user storage cost by manual byte math:"); println!("storage cost: {}", INIT_ACCOUNT_WITH_ONE_KEY_STORAGE); } ``` [`Output`]: Calculating new user storage cost by calling SDK: initial storage: 307200, current storage: 307966, storage cost: 766 Calculating new user storage cost by manual byte math: storage cost: 358

MediumInvalid Request TypeFixed

If the owner or an approver creates an approval request, it will cause an error if they try to process it more than once in the following functions: 1. `approve_approver_request_persmission` 2. `approve_symbol_listing` 3. `approve_symbol_delisting` 4. `approve_withdraw_verify_key` 5. `approve_trade_verify_key` 6. `approve_autorized_access` 7. `new_account_approved` 8. `token_action_approve` The error message instructs the user to create a new request. However, if a new request is created then these functions will panic as there is already such request type in existance. ```rust=365 pub(crate) fn create_approver_request( &mut self, request_type: ApproveRequestType, request: ApproveRequest, ) { if self.approve_requests.contains_key(&request_type) { env::panic_str("This type of request is already in progress"); } self.approve_requests.insert(&request_type, &request); } ```

MediumMissing Cross Account Transfer Callback FunctionFixed

In the `Near` contract, the usage of `Promise` means that the contained operation will be executed asynchronously with respect to the called method. When the above `Promise` at the pointed lines is created and returned, the change in the contract state took already place. However, no callback is specified for such `Promises` so the new state is not reverted in the case in which the `Promise` fails. The pointed codes at L66 within the function `storage_deposit()` creates the `Promise` that transfers `NEAR` refunds. However, if the transfer of the `NEAR` fails, there is no code logic to send an error message to users. In this case, the `NEAR` refunds still remain in the `asset manager` contract and the users' recorded storage fee is not included the part of the refund, however, users are not aware. A similar issue also exists within functions, `storage_withdraw()` and `storage_unregister()`.

MediumMissing Update User LedgerFixed

The contract state data read from `UnorderedMap` is just a copy of data on the Near chain. The changes in that copy data will not automatically synchronize with the on-chain data. So the `UnorderedMap` function `insert()` should be called again if you want to put the new data on the chain. If the operator-manager has approved the key-removal action, the longer-used keys will be deleted from the `user_record` stored in `user_ledger`, an `UnderedMap` structure data. The `user_record` instance is just a copy of the data stored in `UnderedMap`, and after the keys' deletion, the function `insert()` of `UnderedMap` is never called. In this case, the longer-used keys wanted to delete still exist, and the user can still use the no longer-used keys to create a new order and execute it in the `asset_manager` contract.

MediumRemove Withdraw Info For Current PairFixed

In the function, `operator_reject_withdraw_request()`, regardless of the result from calling the function `complete_withdraw`, the `user_withdraw_requests` are completely removed from the respective mapping at Line #95. If the user has more than one `withdraw_requests`, this can result in loss of data. ```rust=80 withdraw_request.complete_withdraw(withdraw_id, request.amount.0); if withdraw_request.is_empty() { self.user_withdraw_requests.remove(&key); } else { self.user_withdraw_requests.insert(&key, &withdraw_request); } Self::emit_event(Event::WithdrawApprove { user: user.clone(), token: token.clone(), amount: request.amount, withdraw_id, is_withdraw_success: false, event_id: Some(request.event_id), }); self.user_withdraw_requests.remove(&key); ``` Only the current withdraw request should be removed according to the logic.

MediumVisibility of `new_withdraw_id()`Fixed

The function `new_withdraw_id()` is called when a transaction user requests a withdrawal. However, this function has public visibility. This can cause the value of `global_withdraw_id` to be modified by any NEAR account. Therefore the exact number of withdrawal operations cannot be read from the log.

LowDuplicate FunctionsFixed

The linked two functions, `assert_authorized()` and `is_authorized()`, have similar functionality with different output values.

Show 10 more

LowDuplicate Variable AssignmentFixed

File: contract.rs Line #305, #312 The variable `balance` is already assigned and checked in Line #305. ```rust=305 let balance = self .user_token_balances .get(&key) .unwrap_or_else(|| env::panic_str("Insufficient token balance")); if self.get_withdraw_unlock_duration_nanos() == 0 { // In this case we will simply check the balance and if it is bigger or equal then amount will perform the transfer let balance = self .user_token_balances .get(&user_token_to_key(&user, &token)) .unwrap_or_default(); ``` It's unnecessary to redefine it in Line #312. --- File: approver.rs Line #21 It's unnecessary to retrieve the approver of caller again, `approver_id` could be reused in Line #21. ```rust=18 let approver_id = self.approvers_ids.get(&caller); assert!( self.approvers_ids.get(&caller).is_some(), "Caller is not set as an approver" ); ```

LowError MessageFixed

File: contract.rs Line #602 In the function `fee_collector_withdraw`, the error message states the following: ```rust=601 if signer_account != self.fee_collector { env::panic_str("caller should be owner or fee collector"); } ``` However, the function only requires that the signer is the fee collector.

LowIncorrect Error MessageFixed

In the function `user_request_withdraw()`, the error message is given by the following lines of code: ```rust let key = user_token_to_key(&user, &token); let balance = self .user_token_balances .get(&key) .unwrap_or_else(|| env::panic_str("Insufficient token balance")); ``` However, the error results if the key does not exist.

LowMissing Emit EventsFixed

In the function `on_fee_collector_withdraw_complete`, the linked statement should emit an events to notify the transfer.

LowMissing Input ValidationFixed

Per the specifications in `owner.rs`, the contract itself nor existing approvers are able to be an owner, an operator manager, an approver or a fee collector. However, the two initialization functions `new()` and `migrate()` of the `asset manager` contract do not check whether the given account is the `asset manager` contract account itself.

LowNeedless BorrowFixed

The expressions highlighted borrow reference with operator `&`, which will be dereferenced by the compiler immediately.\ For more information, we invite the team to check this [reference](https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow).

LowPotential OverflowFixed

The value type of `approver_id` is `u8` and its maximum value is `255`. This means that the maximum number of approvers is `255`. In Rust, integer literals whose type is unconstrained will default to `i32`. So the type of the constant `1` used at L366 and L375 is `i32` and the maximum left shift is `31` bits. An overflow can occur if the left shift exceeds `31` bits. The value of `approver_id` indicates the left shift bits number. Its max value is `255` which is greater than `31`. So an overflow may occur.

LowRedundant CheckFixed

The statements L50-L52 are used to restrict existing approvers from becoming added as a new approvers. However, the `assert_privilages()` function, called before L50, reverts if given approver is an existing approver.

LowRedundant CloneFixed

It is unnecessary to use `clone` as this value is dropped without further use.

LowTest Function OnlyFixed

The function `set_max_operator_downtime()` is only used for testing.

Show 6 more

LowThe New Approver Can Be OwnerFixed

According to the specifications of the function `set_approver`, the new `approver` cannot be the current `owner` of the `asset manager` contract. However, there is no logic in the function to enforce that the new approver account is not the `owner'.

LowTyposFixed

The following lines of code have a typos: - In Owner.rs ```rust pub(crate) fn assert_privilages ``` - In Approver.rs ```rust pub fn approve_approver_request_persmission(&mut self) ```

LowUnnecessary `let` BindingFixed

Variable assignment is extraneous when returning the result of a `let` binding from a block. Removing the `let` binding would make the code follow the rust coding conventions.\ For example: ```rust=64 let public_key = hex::decode(public_key_hex) .ok() .filter(|key| key.len() == 64) .unwrap_or_else(|| env::panic_str("invalid public key")); public_key ``` can be ```rust=64 hex::decode(public_key_hex) .ok() .filter(|key| key.len() == 64) .unwrap_or_else(|| env::panic_str("invalid public key")) ```

LowUnnecessary Memory Copy OperationsFixed

The values of `symbol` and `account_ids` are `String` type which indicates that their values actually stored in heap not stack. The calling of the `clone()` function causes a deep copy of heap data which is a time consuming operation. The linked statement is just used to ensure that `passed_symbol/passed_account_ids` and `symbol/account_ids` have the same value. It is better that using the the dereference operator `*` to avoid the deep copy of heap data.

LowUnused CodeFixed

The function `assert_self` is defined but is unused.

LowUseless ConversionFixed

It's unnecessary to convert the variable `amount` to the same type.