Sarson / Weavechain - csprUSD - Casper

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.

Code Location

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.

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by substituting PublicKey with Key in both the blacklist and un_blacklist functions.

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.

Code Location

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
}

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.

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.

Code Location

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.

Remediation Plan

RISK ACCEPTED: The Sarson Funds team accepted the risk for this issue.

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.

Code Location

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.

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by reverting with the error CsprUSDError::AlreadyBlacklisted if the address to be blacklisted was already blacklisted.

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.

Code Location

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,
    }));
}

Remediation Plan

ACKNOWLEDGED: The Sarson Funds acknowledged this issue.

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.

Code Location

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.

Remediation Plan

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.

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.

Code Location

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.

Remediation Plan

SOLVED: The Sarson Funds team successfully resolved the issue by implementing the recommended adjustments.

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.

Code Location

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.

Remediation Plan

ACKNOWLEDGED: The Sarson Funds acknowledged this issue.

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.

Code Location

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"

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.

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.

Code Location

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.

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by removing the update_minter_allowance function.

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.

Code Location

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; }

Remediation Plan

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.

Open TODOs can point to architecture or programming issues that still need to be resolved.

Code Location

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 works

The 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.

Remediation Plan

SOLVED: The Sarson Funds team resolved the issue by addressing and implementing the previously pending TODO comments.

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.

Code Location

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,
    }));
}

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by applying the recommended changes.

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.

Code Location

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.

Remediation Plan

SOLVED: The Sarson Funds team solved the issue by removing the migrate entry point.