Bridge Contracts - Casper Association

The endpoint function in the common/bridge_utils/src/utils.rs file is used to create each EntryPoint object during contract deployment. The problem is that this function automatically sets the access permissions of the entry point to Public by default.

In the deploy function, a user group is created to prevent the constructor entry point from being called from an external source. The constructor entry point initializes all the main parameters of the contract, so it should be executed only once.

However, because the endpoint function sets the EntryPointAccess parameter for the constructor entry point to Public, the user group creation becomes ineffective.

Thankfully, this issue is mitigated by the init functions that create the dictionaries during the constructor execution, as they revert execution if the dictionary already exists.

Code Location

Code snippet of endpoint function from common/bridge_utils/src/utils.rs file for EntryPoint object creation:

pub fn endpoint(name: &str, param: Vec<Parameter>) -> EntryPoint {
    EntryPoint::new(
        String::from(name),
        param,
        CLType::Unit,
        EntryPointAccess::Public,
        EntryPointType::Contract,
    )
}

Code snippet of get_entry_points function from contracts/bridge/src/bridge_contract.rs file for constructor entry point creation:

    entry_points.add_entry_point(endpoint(
        "constructor",
        vec![
            Parameter::new("version", u8::cl_type()),
            Parameter::new("role_manager", AccountHash::cl_type()),
            Parameter::new("oracle", Oracle::cl_type()),
            Parameter::new("unlock_signer", AccountHash::cl_type()),
            Parameter::new("fee_collector", AccountHash::cl_type()),
            Parameter::new("base_fee_rate_bp", U256::cl_type()),
        ],
    ));

It is recommended to modify the EntryPointAccess parameter of the constructor entry point and set it to Groups.

Alternatively, since the dictionary initialization already prevents a second execution, the user group can be removed from the deploy function, which would save some gas during deployment.

Remediation Plan

RISK ACCEPTED: The AllBridge team has accepted the risk of this finding.

The base_fee_rate_bp parameter is a fee rate used to calculate the amount of fee collected during the lock operation.

As a general best practice in such operations, any rate corresponding to a fee should be checked to ensure it is less than 100% or below a fixed maximum threshold (e.g., 20%).

In this case, the base_fee_rate_bp value is not validated in either the set_base_fee_rate or the init functions. Although any value over 100% would result in the BridgeError::AmountTooSmall error, a 100% value would be allowed, which could leave the amount_to_lock equal to zero, for example.

Code Location

Code snippet of set_base_fee_rate function from contract/bridge/src/bridge_mics.rs file:

    pub fn set_base_fee_rate(base_fee_rate: U256) -> ContractResult<()> {
        Manager::assert_role(Manager::Bridge)?;

        Bridge::set_base_fee_rate_bp(base_fee_rate)
    }

Code snippet of init function from contract/bridge/src/bridge.rs file for contract initialization:

        Self::set_status(true)?;
        Self::set_version(version)?;
        Self::set_oracle(oracle)?;
        Self::set_fee_collector(fee_collector)?;
        Self::set_unlock_signer(unlock_signer)?;
        Self::set_package_hash(package_hash)?;
        Self::set_base_fee_rate_bp(base_fee_rate_bp)?;

        Ok(())
    }

It is recommended to validate the base_rate_fee_bp parameter before storing it, checking that it is less than 100% or another type of maximum threshold (also less than 100%).

Remediation Plan

RISK ACCEPTED: The AllBridge team has accepted the risk of this finding.

The current ERC20 token contract lacks a mechanism for future upgrades, which is essential for adapting to changes and improvements.

Casper supports contract versioning, allowing developers to deploy new versions of contracts while maintaining compatibility with existing states. This feature is crucial for the long-term evolution and security of smart contracts, enabling seamless updates and continuous improvement without disrupting existing functionality.

It is recommended to implement an upgrade function to enable the contract to be updated to new versions, ensuring ongoing maintenance and flexibility for future enhancements.

Remediation Plan

RISK ACCEPTED: The AllBridge team has accepted the risk of this finding.

The lock function allows a user to create a lock on the Casper blockchain by sending the LockArgs parameters and some funds.

The only checks performed by the contract are to validate if the lock_id has already been registered and if the funds are sufficient to cover the fee. If a malicious transaction is made before the legitimate one with the same lock_id and an amount sufficient to cover the fees, the legitimate lock would be rejected because the lock_id already exists, resulting in a DoS attack.

The sender of the lock transaction is not verified at any point. Although this is a very unlikely scenario because the lock_id is supposed to be a random identifier, it is still possible.

Code Location

Code snippet of _create_lock function from contract/bridge/src/validator.rs file:

        let locks_dict = Locks::instance()?;

        assert_lock_id_version(new_lock.lock_id.as_u128(), Self::get_version()?)?;

        assert(
            !locks_dict.has(&new_lock.lock_id),
            BridgeError::LockAlreadyExists,
        )?;

        let sender = runtime::get_caller();

        let lock = Lock {
            sender,
            recipient: new_lock.recipient,
            amount: to_system_precision(amount_to_lock, token_info.precision),
            destination: new_lock.destination,
            token_source: token_info.token_source,
            token_source_address: token_info.token_source_address,
        };

It is recommended to store the lock_id in conjunction with the sender's address as the key in the Locks dictionary to prevent any possibility of lock record falsification.

Remediation Plan

RISK ACCEPTED: The AllBridge team has accepted the risk of this finding.

The transfer_ownership function from ERC20 contract transfers ownership in one step.

In case of an error in the minter input, the contract would be locked in another account without access.

Code Location

Code snippet of transfer_ownership function from contracts/erc20/src/lib.rs file:

    /// Transfers ownership of the contract to a new account (`minter`).
    /// Can only be called by the current minter.
    pub fn transfer_ownership(&mut self, minter: Address) {
        self.assert_minter();

        let minter_uref = *self.minter_uref.get_or_init(minter::minter_uref);
        minter::write_minter_to(minter_uref, minter)
    }

It is recommended to perform this action in two steps: one to establish the new owner's address (propose_new_owner) and one to accept it (accept_ownership).

The latter function can only be executed by the new owner.

Remediation Plan

RISK ACCEPTED: The AllBridge team has accepted the risk of this finding.

The current ERC20 contract used for fungible tokens it is based on the erc20-guide-extraction repository from Casper Network GitHub, and it has not been updated for 2 years.

To ensure enhanced security and maintenance, it is recommended to transition to the Casper CEP-18 standard, which is actively maintained and audited by the Casper community, and serves as the standard for fungible tokens in Casper.

It is recommended to transition to the Casper CEP-18 standard, which serves as the standard for fungible tokens in Casper.

This update will align the token implementation with the latest industry standards and best practices.

Remediation Plan

ACKNOWLEDGED: The AllBridge team has acknowledged this finding.

The create_unlock function contains code that is already present in the unlock function, leading to redundancy and increasing gas usage in transactions and deployment.

Code Location

Code snippet of create_unlock function from contract/bridge/src/validator.rs file:

    pub fn create_unlock(unlock_args: &UnlockArgs) -> ContractResult<()> {
        assert_lock_id_version(unlock_args.lock_id.as_u128(), Self::get_version()?)?;
        let unlock_id = compose_unlock_id(&unlock_args.lock_source, unlock_args.lock_id.as_u128());

        assert_unlock_not_exists(&unlock_id)?;

        Unlocks::instance()?.set(unlock_id);

        Ok(())
    }

Code snippet of unlock function from contract/bridge/src/bridge_unlock.rs file:

    pub fn unlock(unlock_args: UnlockArgs) -> ContractResult<()> {
        Self::assert_active()?;
        assert_lock_id_version(unlock_args.lock_id.as_u128(), Self::get_version()?)?;

        Self::verify_signature(&unlock_args)?;

        let (token_info, token_address) = get_assert_token_info_by_source(
            &unlock_args.token_source,
            &unlock_args.token_source_address,
        )?;

        let amount_total = from_system_precision(unlock_args.amount, token_info.precision);

        let fee = if runtime::get_caller() == Self::get_unlock_signer()? {
            token_info.min_fee
        } else {
            U256::zero()
        };

        let unlock_id = compose_unlock_id(&unlock_args.lock_source, unlock_args.lock_id.as_u128());
        assert(
            !Unlocks::instance()?.has(unlock_id),
            BridgeError::AlreadyReceived,
        )?;

To optimize gas consumption, it is recommended to incorporate the line Unlocks::instance()?.set(unlock_id); within the unlock function and to remove the create_unlock one.

Remediation Plan

ACKNOWLEDGED: The AllBridge team has acknowledged this finding.

The constructor EntryPoint object is not properly created because the corresponding function takes the input argument contract_package_hash, which is not declared in the EntryPoint input parameters.

Code Location

Code snippet of get_entry_points function from contract/bridge/bin/bringe_contract.rs file:

    entry_points.add_entry_point(endpoint(
        "constructor",
        vec![
            Parameter::new("version", u8::cl_type()),
            Parameter::new("role_manager", AccountHash::cl_type()),
            Parameter::new("oracle", Oracle::cl_type()),
            Parameter::new("unlock_signer", AccountHash::cl_type()),
            Parameter::new("fee_collector", AccountHash::cl_type()),
            Parameter::new("base_fee_rate_bp", U256::cl_type()),
        ],
    ));

Code snippet of constructor function from contract/bridge/bin/bridge_contract.rs file:

fn constructor() {
    Bridge::init(
        runtime::get_named_arg("version"),
        runtime::get_named_arg("role_manager"),
        runtime::get_named_arg("oracle"),
        runtime::get_named_arg("unlock_signer"),
        runtime::get_named_arg("fee_collector"),
        runtime::get_named_arg("contract_package_hash"),
        runtime::get_named_arg("base_fee_rate_bp"),
    )
    .unwrap_or_revert();
}

It is recommended to rewrite the declaration of the EntryPoint object to include the contract_package_hash variable. However, this is not critical as calls to entry points work with extra parameters in addition to those declared.

Remediation Plan

ACKNOWLEDGED: The AllBridge team has acknowledged this finding.