Prepared by:
HALBORN
Last Updated 05/22/2024
Date of Engagement by: April 12th, 2024 - April 22nd, 2024
100% of all REPORTED Findings have been addressed
All findings
15
Critical
0
High
0
Medium
1
Low
3
Informational
11
Sarson Funds engaged Halborn to conduct a security assessment of the csprUSD contract which is a fork of the existing CEP-18 standard implementation, a Casper-equivalent of Ethereum's ERC-20 standard, beginning on April 12th, 2024 and ending on April 22nd, 2024. This security assessment was scoped to the smart contracts in the csprUSD GitHub repository.
The team at Halborn was provided 1 week and 1 day for the engagement and assigned a full-time security engineer to check the security of the smart contract. The security engineer is a blockchain and smart-contract security expert with advanced penetration testing and smart-contract hacking skills, and deep knowledge of multiple blockchain protocols.
The purpose of this assessment is to:
Ensure that smart contract functions operate as intended
Identify potential security issues with the smart contracts
In summary, Halborn identified some improvements to reduce the likelihood and impact of risks, many of which were effectively resolved by the Sarson Funds team. The main ones were the following:
Store blacklisted addresses under a Dictionary.
Change the blacklisting mechanism to support all types of accounts by using Key instead of PublicKey.
Add the missing decimals entry point definition to the csprUSD contract's entry points.
Halborn performed a combination of the manual view of the code and automated security testing to balance efficiency, timeliness, practicality, and accuracy regarding the scope of the smart contract assessment. While manual testing is recommended to uncover flaws in logic, process, and implementation, automated testing techniques help enhance the coverage of smart contracts. They can quickly identify items that do not follow security best practices. The following phases and associated tools were used throughout the term of the assessment:
Research into architecture, purpose, and use of the platform.
Manual code read and walkthrough.
Manual Assessment of use and safety for the critical Rust variables and functions in scope to identify any arithmetic related vulnerability classes.
Race condition tests.
Cross contract call controls.
Architecture related logical controls.
Scanning of Rust files for vulnerabilities.(cargo audit)
Checking the unsafe code usage. (cargo-geiger)
| EXPLOITABILIY METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Attack Origin (AO) | Arbitrary (AO:A) Specific (AO:S) | 1 0.2 |
| Attack Cost (AC) | Low (AC:L) Medium (AC:M) High (AC:H) | 1 0.67 0.33 |
| Attack Complexity (AX) | Low (AX:L) Medium (AX:M) High (AX:H) | 1 0.67 0.33 |
| IMPACT METRIC () | METRIC VALUE | NUMERICAL VALUE |
|---|---|---|
| Confidentiality (C) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Integrity (I) | None (I:N) Low (I:L) Medium (I:M) High (I:H) Critical (I:C) | 0 0.25 0.5 0.75 1 |
| Availability (A) | None (A:N) Low (A:L) Medium (A:M) High (A:H) Critical (A:C) | 0 0.25 0.5 0.75 1 |
| Deposit (D) | None (D:N) Low (D:L) Medium (D:M) High (D:H) Critical (D:C) | 0 0.25 0.5 0.75 1 |
| Yield (Y) | None (Y:N) Low (Y:L) Medium (Y:M) High (Y:H) Critical (Y:C) | 0 0.25 0.5 0.75 1 |
| SEVERITY COEFFICIENT () | COEFFICIENT VALUE | NUMERICAL VALUE |
|---|---|---|
| Reversibility () | None (R:N) Partial (R:P) Full (R:F) | 1 0.5 0.25 |
| Scope () | Changed (S:C) Unchanged (S:U) | 1.25 1 |
| Severity | Score Value Range |
|---|---|
| Critical | 9 - 10 |
| High | 7 - 8.9 |
| Medium | 4.5 - 6.9 |
| Low | 2 - 4.4 |
| Informational | 0 - 1.9 |
Critical
0
High
0
Medium
1
Low
3
Informational
11
| Security analysis | Risk level | Remediation Date |
|---|---|---|
| Using blacklisted vector as a NamedKey may lead to breaking the blacklisting mechanism and causing excessive fees for core functionalities | Medium | Solved - 04/25/2024 |
| Inadequate blacklisting mechanism fails to support contracts | Low | Solved - 04/24/2024 |
| Excluding decimals entry point definition will lead to the function's DoS | Low | Solved - 04/29/2024 |
| Lack of the two step ownership transfer pattern | Low | Risk Accepted |
| Flawed blacklisting mechanism may lead to the occurrence of duplicate entries for the same blacklisted account | Informational | Solved - 04/24/2024 |
| The new master minter does not possess minter privileges | Informational | Acknowledged |
| Neglecting to store the package hash under the contract's named keys results in non-compliance with the CEP18 standard | Informational | Solved - 04/29/2024 |
| Mismatched argument names and types leading to inconsistencies in entry points definitions and their implementations | Informational | Solved - 04/30/2024 |
| Missing zero address checks | Informational | Acknowledged |
| Misleading errors descriptions | Informational | Solved - 04/29/2024 |
| Presence of two equivalent functions | Informational | Solved - 04/29/2024 |
| Short-circuiting is essential for optimizing gas usage | Informational | Solved - 04/29/2024 |
| Presence of TODOs implying unfinished code | Informational | Solved - 05/01/2024 |
| Redundant checks | Informational | Solved - 04/29/2024 |
| Unused code | Informational | Solved - 04/29/2024 |
// Medium
Dictionaries are ideal for storing larger volumes of data for which NamedKeys would be less suitable.
According to Casper's documentation: "Dictionaries allow a contract to store additional data without drastically expanding the size of the NamedKeys within their context. If a contract's NamedKeys expand too far, they may run into system limitations that would unintentionally disable the contract's functionality."
The current blacklisting mechanism operates by using a NamedKey that holds a vector containing all the blacklisted accounts. This approach may lead to significant inefficiencies, particularly as the size of the vector grows substantially, resulting in heightened gas consumption. As a result, it will break the blacklisting functionality and affect core features when using the is_blacklisted_util check due to extremely high gas usage.
The install_contract function in the csprUSD contract stores a vector containing the blacklisted addresses public keys under the blacklisted NamedKey:
pub fn install_contract() {
let name: String = runtime::get_named_arg(NAME);
let symbol: String = runtime::get_named_arg(SYMBOL);
let currency: String = runtime::get_named_arg(CURRENCY);
let decimals: u8 = runtime::get_named_arg(DECIMALS);
let master_minter: Key = runtime::get_named_arg(MASTER_MINTER);
let pauser: Key = runtime::get_named_arg(PAUSER);
let blacklister: PublicKey = runtime::get_named_arg(BLACKLISTER);
let owner: Key = runtime::get_named_arg(OWNER);
let mut named_keys = NamedKeys::new();
named_keys.insert(NAME.to_string(), storage::new_uref(name).into());
named_keys.insert(SYMBOL.to_string(), storage::new_uref(symbol).into());
named_keys.insert(CURRENCY.to_string(), storage::new_uref(currency).into());
named_keys.insert(DECIMALS.to_string(), storage::new_uref(decimals).into());
named_keys.insert(
MASTER_MINTER.to_string(),
storage::new_uref(master_minter).into(),
);
named_keys.insert(IS_PAUSED.to_string(), storage::new_uref(false).into());
named_keys.insert(PAUSER.to_string(), storage::new_uref(pauser).into());
named_keys.insert(
BLACKLISTER.to_string(),
storage::new_uref(blacklister).into(),
);
named_keys.insert(OWNER.to_string(), storage::new_uref(owner).into());
named_keys.insert(
TOTAL_SUPPLY.to_string(),
storage::new_uref(U256::zero()).into(),
);
let blacklisted_init_value: Vec<PublicKey> = Vec::new();
named_keys.insert(
BLACKLISTED.to_string(),
storage::new_uref(blacklisted_init_value).into(),
);
let entry_points = generate_entry_points();
let (contract_hash, contract_version) = storage::new_contract(
entry_points,
Some(named_keys),
Some(CONTRACT_PACKAGE_HASH.to_string()),
Some(CONTRACT_ACCESS.to_string()),
);
let package_hash = runtime::get_key(CONTRACT_PACKAGE_HASH).unwrap_or_revert();
// Store contract_hash and contract_version under the keys CONTRACT_NAME and CONTRACT_VERSION
runtime::put_key(CONTRACT_HASH, contract_hash.into());
runtime::put_key(CONTRACT_PACKAGE_HASH, package_hash);
runtime::put_key(CONTRACT_VERSION, storage::new_uref(contract_version).into());
// Call contract to initialize it
let init_args = runtime_args! { MASTER_MINTER => master_minter};
runtime::call_contract::<()>(contract_hash, INIT_ENTRY_POINT_NAME, init_args);
}The blacklisting::is_blacklisted_util function, which is used in the csprUSD contract's core functions, reads the blacklisted NamedKey containing all the blacklisted public keys and returns whether the given account is blacklisted:
pub(crate) fn is_blacklisted_util(account: Key) -> bool {
let currently_blacklisted: Vec<PublicKey> = read_from(BLACKLISTED);
if currently_blacklisted
.iter()
.any(|value| Key::Account((*value).to_account_hash()) == account)
{
return true;
}
false
}The above-mentioned is_blacklisted_util function was used in the following core functions: is_blacklisted, approve, decrease_allowance, increase_allowance, transfer, transfer_from, mint and burn. Below is an example of the is_blacklisted_util function usage inside the transfer function:
#[no_mangle]
pub extern "C" fn transfer() {
when_not_paused();
let sender: Key = utils::get_immediate_caller_address().unwrap_or_revert();
let recipient: Key = runtime::get_named_arg(RECIPIENT);
if is_blacklisted_util(sender) || is_blacklisted_util(recipient) {
revert(CsprUSDError::BlackListedAccount);
}
if sender == recipient {
revert(CsprUSDError::CannotTargetSelfUser);
}
let amount: U256 = runtime::get_named_arg(AMOUNT);
transfer_balance(sender, recipient, amount).unwrap_or_revert();
events::emit_event(Event::Transfer(Transfer {
sender,
recipient,
amount,
}));
}The blacklisting::blacklist_pubkey and blacklisting::un_blacklist_address functions which are used for blacklisting and un-blacklisting, read the whole vector, add or remove an entry, then store the updated vector:
pub(crate) fn blacklist_pubkey(pubkey: PublicKey) {
let mut currently_blacklisted: Vec<PublicKey> = read_from(BLACKLISTED);
currently_blacklisted.push(pubkey);
let uref = get_uref(BLACKLISTED);
storage::write(uref, currently_blacklisted);
}pub(crate) fn un_blacklist_address(pubkey: PublicKey) {
let mut currently_blacklisted: Vec<PublicKey> = read_from(BLACKLISTED);
if let Some(index) = currently_blacklisted
.iter()
.position(|value| *value == pubkey)
{
currently_blacklisted.swap_remove(index);
}
let uref = get_uref(BLACKLISTED);
storage::write(uref, currently_blacklisted);
}The following PoC exploit tries to prove that when the blacklisted vector NamedKey grows to a certain point, the blacklisting feature alongside every core feature that is dependent on will experience very high gas consumption. As a result, users will need to pay very high transactions fees for simple transfer operations.
It's important to note that the blacklisting::blacklist_pubkey() function was edited as follows to make the blacklisted vector large enough to show the appropriate named keys limitation with a much faster test:
pub(crate) fn blacklist_pubkey(pubkey: PublicKey) {
let mut currently_blacklisted: Vec<PublicKey> = read_from(BLACKLISTED);
let uref = get_uref(BLACKLISTED);
// if it's the first time we're calling the blacklist function, push 1230 duplicates of the public key
if currently_blacklisted.is_empty() {
let mut v = Vec::with_capacity(1230);
v.resize(1230, pubkey);
storage::write(uref, v);
} else {
// else only push the given public key
currently_blacklisted.push(pubkey);
storage::write(uref, currently_blacklisted);
}
}The spdlog crate was added as a dependency in the tests/Cargo.toml file. This crate facilitates the generation of clearer logs, enabling us to demonstrate the time taken to blacklist an address as the size of the vector increases.
[dependencies]
# rest of dependencies
spdlog-rs = "0.3"Consider adding the following PoC exploit code into tests/src/pocs/hal_01.rs.
use crate::utility::{
constants::{ACCOUNT_1_ADDR, ARG_RECIPIENT, BLACKLIST, BLACKLISTED, KEY, METHOD_TRANSFER},
installer_request_builders::{setup, TestContext},
};
use casper_engine_test_support::{
ExecuteRequestBuilder, ARG_AMOUNT, DEFAULT_ACCOUNT_ADDR, DEFAULT_PROPOSER_PUBLIC_KEY,
};
use casper_execution_engine::core::{
engine_state::Error as CoreError, execution::Error as ExecError,
};
use casper_types::{runtime_args, Key, PublicKey, RuntimeArgs, U256};
use spdlog::{error, info};
#[test]
fn should_revert_with_out_of_gas_error_if_blacklisted_named_key_is_too_big() {
let (mut builder, TestContext { csprusd_token, .. }) = setup();
// check that no blacklisted accounts exist initially
let blacklisted: Vec<PublicKey> = builder.get_value(csprusd_token, BLACKLISTED);
assert!(blacklisted.is_empty(), "no blacklisted accounts yet");
let mut counter = 0;
loop {
// prepare the blacklist account request
let blacklist_account_request = ExecuteRequestBuilder::contract_call_by_hash(
*ACCOUNT_1_ADDR,
csprusd_token,
BLACKLIST,
runtime_args! {KEY => DEFAULT_PROPOSER_PUBLIC_KEY.clone()},
)
.build();
// execute the blacklist request for DEFAULT_ACCOUNT_PUBLIC_KEY
builder
.exec(blacklist_account_request)
//.expect_success()
.commit();
if builder.is_error() {
//out of gas with a payment of DEFAULT_PAYMENT included, which equals 1_500_000_000_000u64 motes = 1500 cspr = $45
let mut error_msg = builder.get_error().unwrap();
assert!(
matches!(error_msg, CoreError::Exec(ExecError::GasLimit)),
"{:?}",
error_msg
);
error!("blacklist() failed with the given error: {}", error_msg);
info!(
"Last execution gas cost in motes: {}",
builder.last_exec_gas_cost()
);
info!(
"Last execution gas cost in CSPR: {}",
builder.last_exec_gas_cost().value() / 1_000_000_000
);
//try to transfer some tokens -> is_blacklisted check will revert the transaction due to gas
//this happens because the vector is too large and the given addresses are not blacklisted
//as a result, the whole vector will be parsed
let transfer_sender = *DEFAULT_ACCOUNT_ADDR;
let transfer_recipient = *ACCOUNT_1_ADDR;
let csprusd_transfer_args = runtime_args! {
ARG_RECIPIENT => Key::Account(transfer_recipient),
ARG_AMOUNT => U256::one(),
};
let token_transfer_request = ExecuteRequestBuilder::contract_call_by_hash(
transfer_sender,
csprusd_token,
METHOD_TRANSFER,
csprusd_transfer_args,
)
.build();
builder.exec(token_transfer_request).commit();
error_msg = builder.get_error().expect("should have error");
assert!(
matches!(error_msg, CoreError::Exec(ExecError::GasLimit)),
"{:?}",
error_msg
);
error!("transfer() failed with the given error: {}", error_msg);
info!(
"Last execution gas cost in motes: {}",
builder.last_exec_gas_cost()
);
info!(
"Last execution gas cost in CSPR: {}",
builder.last_exec_gas_cost().value() / 1_000_000_000
);
break;
}
info!("Total inserted elements {}", counter + 1230);
counter += 1;
}
}Executing the aforementioned PoC produces the subsequent outcome, revealing that a basic token transfer or blacklisting operation incurs a cost exceeding approximately $45, a price deemed excessively high.
It is suggested to transition to a dictionary-based approach. In this setup, the hashed address serves as the dictionary key, and a boolean value indicates whether the account is blacklisted. This modification offers a more efficient and scalable means to aggregate data over time.
SOLVED: The Sarson Funds team solved the issue by removing the `blacklisted` named key and introducing the following items:
blacklisted_addresses_index: a named key that holds the blacklisted addresses count.
index_to_blacklisted_addr: a dictionary wherein each key represents an index, with the corresponding value being the blacklisted address stored at that specific index.
blacklisted_addr_to_index: a dictionary wherein each key corresponds to a blacklisted address, while the associated value indicates the index where that particular address is stored within the index_to_blacklisted_addr dictionary.
// Low
The implemented blacklisting mechanism only supports public keys. As a result, it will only be able to blacklist EOAs.
Since contracts can be malicious (e.g. MEV bots), it won't be possible to blacklist them.
Consequently, a malevolent user can execute a preemptive transfer of their funds to a malicious contract prior to blacklisting, subsequently channeling them to a newly established account, or conduct operations through a malevolent contract devoid of the threat of blacklisting.
The blacklist function only supports public keys:
#[no_mangle]
pub extern "C" fn blacklist() {
only_blacklister();
let pubkey_to_blacklist: PublicKey = runtime::get_named_arg(KEY);
blacklist_pubkey(pubkey_to_blacklist.clone());
events::emit_event(Event::Blacklisted(Blacklisted {
account: pubkey_to_blacklist,
}));
}Consider changing the blacklisting mechanism to support all types of accounts by using Key instead of PublicKey.
SOLVED: The Sarson Funds team solved the issue by substituting PublicKey with Key in both the blacklist and un_blacklist functions.
// Low
During the creation of a contract, a collection containing the definitions of entry points is provided, enabling the contract to route each transaction to the appropriate entry point after deployment. Consequently, if a transaction attempts to call an entry point that is not recognized by the contract, it will revert with a NoSuchMethod error.
In the case of the csprUSD contract deployment, the decimals entry point definition was omitted. Therefore, any calls to this entry point will fail. Given that csprUSD is intended to function as a stable coin within the Casper Network, it is imperative to add it to existing decentralized exchanges.
As a result, any exchanges that properly utilize token decimals to calculate the correct amounts in and out will not support csprUSD, as each transaction will revert when attempting to retrieve the decimals.
The install_contract function passes the entry points' definitions provided by the entry_points::generate_entry_points function during the contract's creation:
pub fn install_contract() {
let name: String = runtime::get_named_arg(NAME);
let symbol: String = runtime::get_named_arg(SYMBOL);
let currency: String = runtime::get_named_arg(CURRENCY);
let decimals: u8 = runtime::get_named_arg(DECIMALS);
let master_minter: Key = runtime::get_named_arg(MASTER_MINTER);
let pauser: Key = runtime::get_named_arg(PAUSER);
let blacklister: PublicKey = runtime::get_named_arg(BLACKLISTER);
let owner: Key = runtime::get_named_arg(OWNER);
let mut named_keys = NamedKeys::new();
named_keys.insert(NAME.to_string(), storage::new_uref(name).into());
named_keys.insert(SYMBOL.to_string(), storage::new_uref(symbol).into());
named_keys.insert(CURRENCY.to_string(), storage::new_uref(currency).into());
named_keys.insert(DECIMALS.to_string(), storage::new_uref(decimals).into());
named_keys.insert(
MASTER_MINTER.to_string(),
storage::new_uref(master_minter).into(),
);
named_keys.insert(IS_PAUSED.to_string(), storage::new_uref(false).into());
named_keys.insert(PAUSER.to_string(), storage::new_uref(pauser).into());
named_keys.insert(
BLACKLISTER.to_string(),
storage::new_uref(blacklister).into(),
);
named_keys.insert(OWNER.to_string(), storage::new_uref(owner).into());
named_keys.insert(
TOTAL_SUPPLY.to_string(),
storage::new_uref(U256::zero()).into(),
);
let blacklisted_init_value: Vec<PublicKey> = Vec::new();
named_keys.insert(
BLACKLISTED.to_string(),
storage::new_uref(blacklisted_init_value).into(),
);
let entry_points = generate_entry_points();
let (contract_hash, contract_version) = storage::new_contract(
entry_points,
Some(named_keys),
Some(CONTRACT_PACKAGE_HASH.to_string()),
Some(CONTRACT_ACCESS.to_string()),
);
let package_hash = runtime::get_key(CONTRACT_PACKAGE_HASH).unwrap_or_revert();
// Store contract_hash and contract_version under the keys CONTRACT_NAME and CONTRACT_VERSION
runtime::put_key(CONTRACT_HASH, contract_hash.into());
runtime::put_key(CONTRACT_PACKAGE_HASH, package_hash);
runtime::put_key(CONTRACT_VERSION, storage::new_uref(contract_version).into());
// Call contract to initialize it
let init_args = runtime_args! { MASTER_MINTER => master_minter};
runtime::call_contract::<()>(contract_hash, INIT_ENTRY_POINT_NAME, init_args);
}
The entry_points::generate_entry_points function is missing the decimals entry point definition:
/// Returns the default set of cspr USD token entry points.
pub fn generate_entry_points() -> EntryPoints {
let mut entry_points = EntryPoints::new();
entry_points.add_entry_point(init());
entry_points.add_entry_point(name());
entry_points.add_entry_point(symbol());
entry_points.add_entry_point(pauser());
entry_points.add_entry_point(is_paused());
entry_points.add_entry_point(owner());
entry_points.add_entry_point(master_minter());
entry_points.add_entry_point(blacklister());
entry_points.add_entry_point(total_supply());
entry_points.add_entry_point(balance_of());
entry_points.add_entry_point(transfer());
entry_points.add_entry_point(approve());
entry_points.add_entry_point(allowance());
entry_points.add_entry_point(decrease_allowance());
entry_points.add_entry_point(increase_allowance());
entry_points.add_entry_point(transfer_from());
entry_points.add_entry_point(burn());
entry_points.add_entry_point(mint());
entry_points.add_entry_point(pause_contract());
entry_points.add_entry_point(unpause_contract());
entry_points.add_entry_point(update_pauser());
entry_points.add_entry_point(transfer_ownership());
entry_points.add_entry_point(configure_minter());
entry_points.add_entry_point(remove_minter());
entry_points.add_entry_point(minter_allowance());
entry_points.add_entry_point(is_minter());
entry_points.add_entry_point(is_blacklisted());
entry_points.add_entry_point(blacklist());
entry_points.add_entry_point(un_blacklist());
entry_points.add_entry_point(update_blacklister());
entry_points.add_entry_point(update_master_minter());
entry_points
}The following exploit attempts to call the decimals entry point and prove that when being called, it will revert with a NoSuchMethod error.
Consider adding the following PoC exploit code into tests/src/pocs/hal_03.rs.
use crate::utility::installer_request_builders::{setup, TestContext};
use casper_engine_test_support::{ExecuteRequestBuilder, DEFAULT_ACCOUNT_ADDR};
use casper_execution_engine::core::{
engine_state::Error as CoreError, execution::Error as ExecError,
};
use casper_types::{runtime_args, RuntimeArgs};
#[test]
fn should_not_find_decimals_entry_point() {
let (mut builder, TestContext { csprusd_token, .. }) = setup();
// build the decimals entry_point request
let decimals_request = ExecuteRequestBuilder::contract_call_by_hash(
*DEFAULT_ACCOUNT_ADDR,
csprusd_token,
"decimals",
runtime_args! {},
)
.build();
// execute the request
builder.exec(decimals_request).commit();
// expect to get an error
let error = builder.get_error().expect("should have error");
// parse the obtained error which should match the following pattern: ExecError::NoSuchMethod("decimals")
let _decimals_entrypoint_name = String::from("decimals");
assert!(
matches!(
error.clone(),
CoreError::Exec(ExecError::NoSuchMethod(_decimals_entrypoint_name))
),
"{:?}",
error
);
}Executing the aforementioned PoC yields the following outcome:
Consider adding the decimals entry point definition to the csprUSD contract's entry points during its creation. This can be done by updating the entry_points::generate_entry_points() as follows:
/// Returns the default set of cspr USD token entry points.
pub fn generate_entry_points() -> EntryPoints {
let mut entry_points = EntryPoints::new();
entry_points.add_entry_point(init());
entry_points.add_entry_point(name());
entry_points.add_entry_point(symbol());
entry_points.add_entry_point(decimals());
entry_points.add_entry_point(pauser());
entry_points.add_entry_point(is_paused());
entry_points.add_entry_point(owner());
entry_points.add_entry_point(master_minter());
entry_points.add_entry_point(blacklister());
entry_points.add_entry_point(total_supply());
entry_points.add_entry_point(balance_of());
entry_points.add_entry_point(transfer());
entry_points.add_entry_point(approve());
entry_points.add_entry_point(allowance());
entry_points.add_entry_point(decrease_allowance());
entry_points.add_entry_point(increase_allowance());
entry_points.add_entry_point(transfer_from());
entry_points.add_entry_point(burn());
entry_points.add_entry_point(mint());
entry_points.add_entry_point(pause_contract());
entry_points.add_entry_point(unpause_contract());
entry_points.add_entry_point(update_pauser());
entry_points.add_entry_point(transfer_ownership());
entry_points.add_entry_point(configure_minter());
entry_points.add_entry_point(remove_minter());
entry_points.add_entry_point(minter_allowance());
entry_points.add_entry_point(is_minter());
entry_points.add_entry_point(is_blacklisted());
entry_points.add_entry_point(blacklist());
entry_points.add_entry_point(un_blacklist());
entry_points.add_entry_point(update_blacklister());
entry_points.add_entry_point(update_master_minter());
entry_points
}SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.
// Low
The present ownership transfer procedure requires the current owner to invoke transfer_ownership().
This method does not verify that the new owner's address is non-zero before updating the owner's named key with the new address. In the event of an invalid nominated address, there exists a possibility that the owner might unintentionally transfer ownership to an unanticipated address, thereby compromising the integrity of all functions that call the only_owner() check.
The absence of a two-step process for pivotal operations renders them susceptible to errors; if an incorrect address is provided, the new address will promptly assume the responsibilities associated with the new role.
The transfer_ownership function is used to transfer the csprUSD contract's ownership:
#[no_mangle]
pub extern "C" fn transfer_ownership() {
only_owner();
let new_owner: Key = runtime::get_named_arg(NEW);
storage::write(get_uref(OWNER), new_owner);
events::emit_event(Event::OwnershipTransferred(OwnershipTransferred {
new_owner,
}));
}It is advisable to validate the new owner's address to ensure it is not a zero address. Additionally, implementing a two-step process is recommended:
First, the current owner nominates an address, and then the nominated address must call an accept_ownership() function to finalize the ownership transfer. This approach guarantees that the nominated address is valid and exists before ownership is transferred.
Moreover, consideration may be given to introducing a revoke_ownership_transfer() function, which can be invoked by the current owner should they discover that an invalid address was provided.
RISK ACCEPTED: The Sarson Funds team accepted the risk for this issue.
// Informational
The csprUSD contract incorporates a blacklisting mechanism that empowers the blacklister account to either blacklist or un-blacklist specified accounts. Typically, when un-blacklisting an account, it should cease to be blacklisted.
However, the current implementation of the blacklist function adds a new entry for the specified address each time it is invoked. Consequently, when attempting to un-blacklist an account, only the initial entry encountered will be removed. This results in the account remaining blacklisted even after the un-blacklist operation.
The blacklist function uses the entry_points::blacklist_pubkey function to blacklist the given public key:
#[no_mangle]
pub extern "C" fn blacklist() {
only_blacklister();
let pubkey_to_blacklist: PublicKey = runtime::get_named_arg(KEY);
blacklist_pubkey(pubkey_to_blacklist.clone());
events::emit_event(Event::Blacklisted(Blacklisted {
account: pubkey_to_blacklist,
}));
}The entry_points::blacklist_pubkey function does not check whether the provided public key has previously been blacklisted:
pub(crate) fn blacklist_pubkey(pubkey: PublicKey) {
let mut currently_blacklisted: Vec<PublicKey> = read_from(BLACKLISTED);
currently_blacklisted.push(pubkey);
let uref = get_uref(BLACKLISTED);
storage::write(uref, currently_blacklisted);
}The following PoC shows that when blacklisting the same account twice, un-blacklisting it once will not cancel its blacklisted status.
Consider adding the following PoC exploit code into tests/src/pocs/hal_05.rs.
use crate::utility::{
constants::{ACCOUNT_1_ADDR, BLACKLIST, BLACKLISTED, KEY, UN_BLACKLIST},
installer_request_builders::{setup, TestContext},
};
use casper_engine_test_support::{ExecuteRequestBuilder, DEFAULT_ACCOUNT_PUBLIC_KEY};
use casper_types::{bytesrepr::ToBytes, runtime_args, PublicKey, RuntimeArgs};
#[test]
fn should_not_un_blacklist_if_account_was_blacklisted_more_than_once() {
let (mut builder, TestContext { csprusd_token, .. }) = setup();
// check that no blacklisted accounts exist initially
let mut blacklisted: Vec<PublicKey> = builder.get_value(csprusd_token, BLACKLISTED);
assert!(blacklisted.is_empty(), "no blacklisted accounts yet");
// ********* blacklist the account, first iteration *********
// prepare the blacklist account request
let blacklist_account_request = ExecuteRequestBuilder::contract_call_by_hash(
*ACCOUNT_1_ADDR,
csprusd_token,
BLACKLIST,
runtime_args! {KEY => DEFAULT_ACCOUNT_PUBLIC_KEY.clone()},
)
.build();
// execute the blacklist request for DEFAULT_ACCOUNT_PUBLIC_KEY
builder
.exec(blacklist_account_request)
.expect_success()
.commit();
// ********* blacklist the account, second iteration *********
// rebuild the same blacklist account request, because ExecuteRequest type can't be copied
let blacklist_account_request = ExecuteRequestBuilder::contract_call_by_hash(
*ACCOUNT_1_ADDR,
csprusd_token,
BLACKLIST,
runtime_args! {KEY => DEFAULT_ACCOUNT_PUBLIC_KEY.clone()},
)
.build();
// re-execute the blacklist request for the same account
builder
.exec(blacklist_account_request)
.expect_success()
.commit();
blacklisted = builder.get_value(csprusd_token, BLACKLISTED);
// verify that the same account was added twice to the blacklisted named key
assert_eq!(
vec![
blacklisted.get(0).unwrap().to_bytes(),
blacklisted.get(1).unwrap().to_bytes()
],
vec![
DEFAULT_ACCOUNT_PUBLIC_KEY.clone().to_bytes(),
DEFAULT_ACCOUNT_PUBLIC_KEY.clone().to_bytes()
],
"the same account was blacklisted twice"
);
// ********* un-blacklist the account *********
// build the un_blacklist request
let un_blacklist_account_request = ExecuteRequestBuilder::contract_call_by_hash(
*ACCOUNT_1_ADDR,
csprusd_token,
UN_BLACKLIST,
runtime_args! {KEY => DEFAULT_ACCOUNT_PUBLIC_KEY.clone()},
)
.build();
// execute the built request
builder
.exec(un_blacklist_account_request)
.expect_success()
.commit();
// verify that the account still has one entry in the blacklisted vector
blacklisted = builder.get_value(csprusd_token, BLACKLISTED);
assert_eq!(
blacklisted.get(0).unwrap().to_bytes(),
DEFAULT_ACCOUNT_PUBLIC_KEY.clone().to_bytes(),
"the account is still blacklisted"
);
}Executing the aforementioned PoC yields the following outcome:
Verify whether the vector contains the public key entry before appending it.
SOLVED: The Sarson Funds team solved the issue by reverting with the error CsprUSDError::AlreadyBlacklisted if the address to be blacklisted was already blacklisted.
// Informational
A master minter is inherently designated as a minter; without minter status, the ability to mint or burn is not feasible.
However, the implementation of update_master_minter is inadequate as it neglects to designate the new master minter as a minter.
This limitation can be addressed by invoking the configure_minter function with the master minter's own address, thereby minimizing its impact.
The update_master_minter function does not add the new_master_minter to the minters' dictionary:
#[no_mangle]
pub extern "C" fn update_master_minter() {
only_owner();
let new_master_minter: Key = runtime::get_named_arg(NEW);
storage::write(get_uref(MASTER_MINTER), new_master_minter);
events::emit_event(Event::MasterMinterChanged(MasterMinterChanged {
new_master_minter,
}));
}Add the new master minter to the minters dictionary as follows:
#[no_mangle]
pub extern "C" fn update_master_minter() {
only_owner();
let new_master_minter: Key = runtime::get_named_arg(NEW);
storage::write(get_uref(MASTER_MINTER), new_master_minter);
add_minter(new_master_minter);
events::emit_event(Event::MasterMinterChanged(MasterMinterChanged {
new_master_minter,
}));
}ACKNOWLEDGED: The Sarson Funds acknowledged this issue.
// Informational
During the contract's deployment, rather than storing the contract package hash under the contract's named keys, it was inadvertently re-stored under the deployer's named keys, leading to duplicated assignments.
This behavior deviates from the original CEP18 token standard, which mandates that the package hash be exclusively included under the contract's named keys.
The install_contract function stores the contract's package hash under the deployer's named keys through the storage::new_contract function call, then reads the stored package hash and restores it under the deployer's named keys:
pub fn install_contract() {
let name: String = runtime::get_named_arg(NAME);
let symbol: String = runtime::get_named_arg(SYMBOL);
let currency: String = runtime::get_named_arg(CURRENCY);
let decimals: u8 = runtime::get_named_arg(DECIMALS);
let master_minter: Key = runtime::get_named_arg(MASTER_MINTER);
let pauser: Key = runtime::get_named_arg(PAUSER);
let blacklister: PublicKey = runtime::get_named_arg(BLACKLISTER);
let owner: Key = runtime::get_named_arg(OWNER);
let mut named_keys = NamedKeys::new();
named_keys.insert(NAME.to_string(), storage::new_uref(name).into());
named_keys.insert(SYMBOL.to_string(), storage::new_uref(symbol).into());
named_keys.insert(CURRENCY.to_string(), storage::new_uref(currency).into());
named_keys.insert(DECIMALS.to_string(), storage::new_uref(decimals).into());
named_keys.insert(
MASTER_MINTER.to_string(),
storage::new_uref(master_minter).into(),
);
named_keys.insert(IS_PAUSED.to_string(), storage::new_uref(false).into());
named_keys.insert(PAUSER.to_string(), storage::new_uref(pauser).into());
named_keys.insert(
BLACKLISTER.to_string(),
storage::new_uref(blacklister).into(),
);
named_keys.insert(OWNER.to_string(), storage::new_uref(owner).into());
named_keys.insert(
TOTAL_SUPPLY.to_string(),
storage::new_uref(U256::zero()).into(),
);
let blacklisted_init_value: Vec<PublicKey> = Vec::new();
named_keys.insert(
BLACKLISTED.to_string(),
storage::new_uref(blacklisted_init_value).into(),
);
let entry_points = generate_entry_points();
let (contract_hash, contract_version) = storage::new_contract(
entry_points,
Some(named_keys),
Some(CONTRACT_PACKAGE_HASH.to_string()),
Some(CONTRACT_ACCESS.to_string()),
);
let package_hash = runtime::get_key(CONTRACT_PACKAGE_HASH).unwrap_or_revert();
// Store contract_hash and contract_version under the keys CONTRACT_NAME and CONTRACT_VERSION
runtime::put_key(CONTRACT_HASH, contract_hash.into());
runtime::put_key(CONTRACT_PACKAGE_HASH, package_hash);
runtime::put_key(CONTRACT_VERSION, storage::new_uref(contract_version).into());
// Call contract to initialize it
let init_args = runtime_args! { MASTER_MINTER => master_minter};
runtime::call_contract::<()>(contract_hash, INIT_ENTRY_POINT_NAME, init_args);
}Consider adding the following PoC exploit code into tests/src/pocs/hal_07.rs.
use crate::utility::installer_request_builders::{setup, TestContext};
use casper_engine_test_support::DEFAULT_ACCOUNT_ADDR;
use casper_types::Key;
pub const CONTRACT_PACKAGE_HASH: &str = "csprUSD_contract_package_hash";
#[test]
#[should_panic = "should have named key"]
fn should_not_store_contract_package_hash_under_contract_after_install() {
let (mut builder, TestContext { csprusd_token, .. }) = setup();
// the following will panic because csprUSD contract didn't store its package hash under its named keys
builder.get_value::<Key>(csprusd_token, CONTRACT_PACKAGE_HASH);
}
#[test]
fn should_store_contract_package_hash_under_account_after_install() {
let (builder, _contract_hash) = setup();
let account = builder
.get_account(*DEFAULT_ACCOUNT_ADDR)
.expect("should have account");
let named_keys = account.named_keys();
// the deployer has the csprUSD contract package hash under its own named keys
// this will also pass if we comment the following line:
// https://github.com/Sarson-Funds/csprUSD/blob/2a07b39bebfb970f7d0f5686c9783ddb2c5b07bd/csprusd/src/main.rs#L575
// because the package hash was already stored under the deployer's named keys through the `storage::new_contract` call
assert!(
named_keys.contains_key(CONTRACT_PACKAGE_HASH),
"{:?}",
named_keys
);
}Executing the aforementioned PoC yields the following outcome:
To ensure proper storage of the package hash, include it as a runtime argument for the init entry point and store it within the init function. By doing so, the package hash will be appropriately stored under the correct context's named keys, specifically within the csprUSD contract.
This approach is crucial, as the csprUSD contract serves as the direct caller when initiating external calls using runtime::call_contract.
SOLVED: The Sarson Funds team solved the issue by applying the recommended changes and storing the contract's package hash under the package_hash named key within the csprUSD contract.
// Informational
Across the codebase, inconsistencies arise where the types and names of entry point arguments in the implementation within main.rs do not align with the corresponding values specified in the entry points definition found in entry_points.rs.
The affected entry points include: update_pauser, update_master_minter, is_blacklisted, blacklist, un_blacklist, transfer_ownership, configure_minter, remove_minter, minter_allowance, is_minter and init.
The update_pauser function's entry point definition states that the parameter name is pauser and the parameter type is AccountHash:
pub fn update_pauser() -> EntryPoint {
EntryPoint::new(
String::from(UPDATE_PAUSER_ENTRY_POINT_NAME),
vec![Parameter::new(PAUSER, AccountHash::cl_type())],
CLType::Unit,
EntryPointAccess::Public,
EntryPointType::Contract,
)
}The update_pauser function's implementation states that the parameter name is new and the parameter type is Key:
#[no_mangle]
pub extern "C" fn update_pauser() {
only_owner();
let new_pauser: Key = runtime::get_named_arg(NEW);
storage::write(get_uref(PAUSER), new_pauser);
events::emit_event(Event::PauserChanged(NewPauser { new_pauser }));
}It is advisable to ensure uniformity by employing identical argument names and types in both the entry points definitions and their respective implementations.
SOLVED: The Sarson Funds team successfully resolved the issue by implementing the recommended adjustments.
// Informational
The csprUSD contract lacks address validation both during deployment and within its setter functions.
This oversight allows for the configuration of a zero address, potentially leading to complications during execution.
For instance, passing Key::Account(AccountHash::default()) to the install_contract function as an owner address would render it impossible to alter this address in subsequent operations.
Consider the install_contract function as an example, which fails to validate whether the provided addresses are non-zero addresses.
pub fn install_contract() {
let name: String = runtime::get_named_arg(NAME);
let symbol: String = runtime::get_named_arg(SYMBOL);
let currency: String = runtime::get_named_arg(CURRENCY);
let decimals: u8 = runtime::get_named_arg(DECIMALS);
let master_minter: Key = runtime::get_named_arg(MASTER_MINTER);
let pauser: Key = runtime::get_named_arg(PAUSER);
let blacklister: PublicKey = runtime::get_named_arg(BLACKLISTER);
let owner: Key = runtime::get_named_arg(OWNER);
let mut named_keys = NamedKeys::new();
named_keys.insert(NAME.to_string(), storage::new_uref(name).into());
named_keys.insert(SYMBOL.to_string(), storage::new_uref(symbol).into());
named_keys.insert(CURRENCY.to_string(), storage::new_uref(currency).into());
named_keys.insert(DECIMALS.to_string(), storage::new_uref(decimals).into());
named_keys.insert(
MASTER_MINTER.to_string(),
storage::new_uref(master_minter).into(),
);
named_keys.insert(IS_PAUSED.to_string(), storage::new_uref(false).into());
named_keys.insert(PAUSER.to_string(), storage::new_uref(pauser).into());
named_keys.insert(
BLACKLISTER.to_string(),
storage::new_uref(blacklister).into(),
);
named_keys.insert(OWNER.to_string(), storage::new_uref(owner).into());
named_keys.insert(
TOTAL_SUPPLY.to_string(),
storage::new_uref(U256::zero()).into(),
);
let blacklisted_init_value: Vec<PublicKey> = Vec::new();
named_keys.insert(
BLACKLISTED.to_string(),
storage::new_uref(blacklisted_init_value).into(),
);
let entry_points = generate_entry_points();
let (contract_hash, contract_version) = storage::new_contract(
entry_points,
Some(named_keys),
Some(CONTRACT_PACKAGE_HASH.to_string()),
Some(CONTRACT_ACCESS.to_string()),
);
let package_hash = runtime::get_key(CONTRACT_PACKAGE_HASH).unwrap_or_revert();
// Store contract_hash and contract_version under the keys CONTRACT_NAME and CONTRACT_VERSION
runtime::put_key(CONTRACT_HASH, contract_hash.into());
runtime::put_key(CONTRACT_PACKAGE_HASH, package_hash);
runtime::put_key(CONTRACT_VERSION, storage::new_uref(contract_version).into());
// Call contract to initialize it
let init_args = runtime_args! { MASTER_MINTER => master_minter};
runtime::call_contract::<()>(contract_hash, INIT_ENTRY_POINT_NAME, init_args);
}Consider validating that the provided addresses are not zero addresses.
ACKNOWLEDGED: The Sarson Funds acknowledged this issue.
// Informational
For each variant of the CsprUSDError, a concise description was provided to offer additional context for each error.
However, it was observed that the descriptions were misleading in two instances.
The highlighted error descriptions within csprusd/src/error.rs are misleading:
#[repr(u16)]
#[derive(Clone, Copy)]
pub enum CsprUSDError {
/// Contract called from within an invalid context.
InvalidContext = 60000,
/// Spender does not have enough balance.
InsufficientBalance = 60001,
/// Spender does not have enough allowance approved.
InsufficientAllowance = 60002,
/// Operation would cause an integer overflow.
Overflow = 60003,
/// A required package hash was not specified.
PackageHashMissing = 60004,
/// The package hash specified does not represent a package.
PackageHashNotPackage = 60005,
/// An unknown error occurred.
Phantom = 60008,
/// Failed to read the runtime arguments provided.
FailedToGetArgBytes = 60009,
/// The flag to enable the mint and burn mode is invalid.
InvalidEnableMBFlag = 60014,
/// This contract instance cannot be initialized again.
AlreadyInitialized = 60015,
/// The mint and burn mode is disabled.
CannotTargetSelfUser = 60017,
/// Contract is currently paused, not allowed to progress
ContractPaused = 65000,
/// Operation disallowed because account is not the pauser
NotPauser = 65001,
/// Pauser must be provided on contract initialization
NoPauserProvided = 65002,
/// Owner must be provided on contract initialization
NoOwnerProvided = 65003,
/// Operation disallowed because account is not the owner
NotOwner = 65004,
/// Operation disallowed because account is not a minter account
NotMinter = 65005,
/// Operation disallowed because account is blacklisted
BlackListedAccount = 65006,
/// Can't proceed because amount in the request exceeds the account's mint allowance
ExceedsMintAllowance = 65007,
/// Error while calling into Casper for creating dictionary
FailedToCreateDictionary = 65011,
/// Minting negative amounts is not allowed
CannotMintZeroAmount = 65012,
/// Operation disallowed because account is not the master minter
NotMasterMinter = 65013,
/// Operation disallowed because account is not the blacklister
NotBlacklister = 65014,
/// One can only burn a positive amount of tokens
CannotBurnZeroAmount = 65015,
/// An account can't burn more than its balance
BurnExceedsBalance = 65016,
}Considering that unsigned integers are utilized, where the minimum value is zero, representing a strictly positive value, it is recommended to amend these descriptions as follows:
"Minting negative amounts is not allowed" -> "Minting zero amounts is not allowed"
"One can only burn a positive amount of tokens" -> "One can only burn a strictly positive amount of tokens"
SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.
// Informational
Both the set_minter_allowed and update_minter_allowance functions exhibit identical logic.
This redundancy contributes to an unnecessarily expanded contract size and increased gas consumption.
The set_minter_allowed and update_minter_allowance functions exhibit identical logic:
pub(crate) fn update_minter_allowance(minter: Key, updated_allowance: U256) {
let dict_seed = get_uref(MINTER_ALLOWED);
let dict_key = hex::encode(runtime::blake2b(minter.to_bytes().unwrap_or_revert()));
storage::dictionary_put(dict_seed, &dict_key, updated_allowance);
}
pub(crate) fn set_minter_allowed(minter: Key, minter_allowed_amount: U256) {
let dict_seed = get_uref(MINTER_ALLOWED);
let dict_key = hex::encode(runtime::blake2b(minter.to_bytes().unwrap_or_revert()));
storage::dictionary_put(dict_seed, &dict_key, minter_allowed_amount);
}Consider removing one of the functions.
SOLVED: The Sarson Funds team solved the issue by removing the update_minter_allowance function.
// Informational
If the amount to be transferred within the transfer_from function equates to zero, it is advisable for the contract to short-circuit to minimize gas consumption.
Otherwise, the contract will expend gas in an attempt to update allowances, execute a transfer of a zero amount, and emit an inconsequential event.
This principle can similarly be applied to the transfer function.
The transfer_from function does not short-circuit when the amount to be transferred equates to zero:
#[no_mangle]
pub extern "C" fn transfer_from() {
when_not_paused();
let spender: Key = utils::get_immediate_caller_address().unwrap_or_revert();
let recipient: Key = runtime::get_named_arg(RECIPIENT);
let owner: Key = runtime::get_named_arg(OWNER);
if is_blacklisted_util(spender) || is_blacklisted_util(recipient) || is_blacklisted_util(owner)
{
revert(CsprUSDError::BlackListedAccount);
}
if owner == recipient {
revert(CsprUSDError::CannotTargetSelfUser);
}
let amount: U256 = runtime::get_named_arg(AMOUNT);
let allowances_uref = get_allowances_uref();
let spender_allowance: U256 = read_allowance_from(allowances_uref, owner, spender);
if spender_allowance < amount {
revert(CsprUSDError::InsufficientAllowance);
}
transfer_balance(owner, recipient, amount).unwrap_or_revert();
let new_spender_allowance = spender_allowance
.checked_sub(amount)
.ok_or(CsprUSDError::InsufficientAllowance)
.unwrap_or_revert();
write_allowance_to(allowances_uref, owner, spender, new_spender_allowance);
events::emit_event(Event::TransferFrom(TransferFrom {
spender,
owner,
recipient,
amount,
}));
}The transfer function does not short-circuit when the amount to be transferred equates to zero:
#[no_mangle]
pub extern "C" fn transfer() {
when_not_paused();
let sender: Key = utils::get_immediate_caller_address().unwrap_or_revert();
let recipient: Key = runtime::get_named_arg(RECIPIENT);
if is_blacklisted_util(sender) || is_blacklisted_util(recipient) {
revert(CsprUSDError::BlackListedAccount);
}
if sender == recipient {
revert(CsprUSDError::CannotTargetSelfUser);
}
let amount: U256 = runtime::get_named_arg(AMOUNT);
transfer_balance(sender, recipient, amount).unwrap_or_revert();
events::emit_event(Event::Transfer(Transfer {
sender,
recipient,
amount,
}));
}It is suggested to incorporate a check on the amount argument and initiate a short circuit promptly if the amount equals zero.
if amount.is_zero() { return; }SOLVED: The Sarson Funds team solved the issue by incorporating a short circuit mechanism which triggers the CsprUSDError::CannotTransferZeroAmount error, whenever an attempt is made to transfer an amount equal to zero.
// Informational
Open TODOs can point to architecture or programming issues that still need to be resolved.
The csprUSD contract's events contain the following TODO comments:
#[derive(Event, Debug, PartialEq, Eq)]
pub struct Pause {} // TODO: is this needed?
#[derive(Event, Debug, PartialEq, Eq)]
pub struct Unpause {} // TODO: is this needed?The tests related to contract's upgrade in tests/src/contract_upgrade.rs contain the following TODO comment:
// TODO: disable old version and prove that it no longer worksThe following TODO comment was identified in tests/Cargo.toml:
# TODO: remove. Used only to reproduce dict key generating from test to access elements of global state with casper-client
hex = { version = "0.4.3", default-features = false }
base64 = { version = "0.20.0", default-features = false, features = ["alloc"] }Consider resolving the TODOs before deploying.
SOLVED: The Sarson Funds team resolved the issue by addressing and implementing the previously pending TODO comments.
// Informational
While requirements are indispensable for ensuring the proper usage and security of contracts, an instance of redundant validation has been identified.
In this scenario, the code validates that the spender possesses sufficient allowance before transferring the specified amount.
Subsequently, it revalidates the same condition when deducting the given amount from the spender's allowance post-transfer.
Consequently, the second verification will never result in a revert and incurs additional gas usage.
The transfer_from function has the following redundant checks:
#[no_mangle]
pub extern "C" fn transfer_from() {
when_not_paused();
let spender: Key = utils::get_immediate_caller_address().unwrap_or_revert();
let recipient: Key = runtime::get_named_arg(RECIPIENT);
let owner: Key = runtime::get_named_arg(OWNER);
if is_blacklisted_util(spender) || is_blacklisted_util(recipient) || is_blacklisted_util(owner)
{
revert(CsprUSDError::BlackListedAccount);
}
if owner == recipient {
revert(CsprUSDError::CannotTargetSelfUser);
}
let amount: U256 = runtime::get_named_arg(AMOUNT);
let allowances_uref = get_allowances_uref();
let spender_allowance: U256 = read_allowance_from(allowances_uref, owner, spender);
if spender_allowance < amount {
revert(CsprUSDError::InsufficientAllowance);
}
transfer_balance(owner, recipient, amount).unwrap_or_revert();
let new_spender_allowance = spender_allowance
.checked_sub(amount)
.ok_or(CsprUSDError::InsufficientAllowance)
.unwrap_or_revert();
write_allowance_to(allowances_uref, owner, spender, new_spender_allowance);
events::emit_event(Event::TransferFrom(TransferFrom {
spender,
owner,
recipient,
amount,
}));
}It is advisable to eliminate the initial validation and relocate the balance transfer after computing the new spender's allowance:
#[no_mangle]
pub extern "C" fn transfer_from() {
when_not_paused();
let spender: Key = utils::get_immediate_caller_address().unwrap_or_revert();
let recipient: Key = runtime::get_named_arg(RECIPIENT);
let owner: Key = runtime::get_named_arg(OWNER);
if is_blacklisted_util(spender) || is_blacklisted_util(recipient) || is_blacklisted_util(owner)
{
revert(CsprUSDError::BlackListedAccount);
}
if owner == recipient {
revert(CsprUSDError::CannotTargetSelfUser);
}
let amount: U256 = runtime::get_named_arg(AMOUNT);
let allowances_uref = get_allowances_uref();
let spender_allowance: U256 = read_allowance_from(allowances_uref, owner, spender);
let new_spender_allowance = spender_allowance
.checked_sub(amount)
.ok_or(CsprUSDError::InsufficientAllowance)
.unwrap_or_revert();
transfer_balance(owner, recipient, amount).unwrap_or_revert();
write_allowance_to(allowances_uref, owner, spender, new_spender_allowance);
events::emit_event(Event::TransferFrom(TransferFrom {
spender,
owner,
recipient,
amount,
}));
}SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.
// Informational
An observation reveals that the migrate entry point has neither been defined nor included in the csprUSD contract's entry point definitions.
Consequently, this entry point will not be acknowledged by the contract and will remain inaccessible from external sources.
Furthermore, the implementation of the entry point is devoid of any logic.
The migrate entry point has no logic:
#[no_mangle]
pub extern "C" fn migrate() {}The csprUSD contract's entry points do not include a definition for the migrate function:
/// Returns the default set of cspr USD token entry points.
pub fn generate_entry_points() -> EntryPoints {
let mut entry_points = EntryPoints::new();
entry_points.add_entry_point(init());
entry_points.add_entry_point(name());
entry_points.add_entry_point(symbol());
entry_points.add_entry_point(pauser());
entry_points.add_entry_point(is_paused());
entry_points.add_entry_point(owner());
entry_points.add_entry_point(master_minter());
entry_points.add_entry_point(blacklister());
entry_points.add_entry_point(total_supply());
entry_points.add_entry_point(balance_of());
entry_points.add_entry_point(transfer());
entry_points.add_entry_point(approve());
entry_points.add_entry_point(allowance());
entry_points.add_entry_point(decrease_allowance());
entry_points.add_entry_point(increase_allowance());
entry_points.add_entry_point(transfer_from());
entry_points.add_entry_point(burn());
entry_points.add_entry_point(mint());
entry_points.add_entry_point(pause_contract());
entry_points.add_entry_point(unpause_contract());
entry_points.add_entry_point(update_pauser());
entry_points.add_entry_point(transfer_ownership());
entry_points.add_entry_point(configure_minter());
entry_points.add_entry_point(remove_minter());
entry_points.add_entry_point(minter_allowance());
entry_points.add_entry_point(is_minter());
entry_points.add_entry_point(is_blacklisted());
entry_points.add_entry_point(blacklist());
entry_points.add_entry_point(un_blacklist());
entry_points.add_entry_point(update_blacklister());
entry_points.add_entry_point(update_master_minter());
entry_points
}Consider either removing or reviewing the logic associated with the migrate entry point.
SOLVED: The Sarson Funds team solved the issue by removing the migrate entry point.
Halborn used automated security scanners to assist with detection of well-known security issues and vulnerabilities. Among the tools used was cargo audit, a security scanner for vulnerabilities reported to the RustSec Advisory Database. All vulnerabilities published in https://crates.io are stored in a repository named The RustSec Advisory Database.
cargo audit is a human-readable version of the advisory database which performs a scanning on Cargo.lock. Security Detections are only in scope. All vulnerabilities shown here were already disclosed in the above report. However, to better assist the developers maintaining this code, the auditors are including the output with the dependencies tree, and this is included in the cargo audit output to better know the dependencies affected by unmaintained and vulnerable crates.
ID | package | Short Description |
|---|---|---|
ed25519-dalek 1.0.1 | Double Public Key Signing Function Oracle Attack on Upgrade to >=2 Which leads to upgrading to casper-types | |
lmdb 0.8.0 | lmdb is unmaintained, use lmdb-rkv instead | |
parity-wasm 0.42.2 | Crate | |
wee_alloc 0.4.5 | wee_alloc is Unmaintained | |
iana-time-zone 0.1.59 | yanked |
Halborn strongly recommends conducting a follow-up assessment of the project either within six months or immediately following any material changes to the codebase, whichever comes first. This approach is crucial for maintaining the project’s integrity and addressing potential vulnerabilities introduced by code modifications.
// Download the full report
* Use Google Chrome for best results
** Check "Background Graphics" in the print settings if needed