Security Vulnerabilities in CashFactory Smart Contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashFactory.sol#L44-L46 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashFactory.sol#L49-L51

Vulnerability details

Impact

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashFactory.sol#L44-L46

bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  bytes32 public constant DEFAULT_ADMIN_ROLE = bytes32(0);

The bug is a security vulnerability because it allows any address to access the MINTER_ROLE, PAUSER_ROLE and DEFAULT_ADMIN_ROLE by simply reading the contract's state. An attacker can use this knowledge to perform malicious actions such as minting new tokens, pausing the contract, and upgrading the logic contract. This can have a significant financial impact on the token holders and the overall integrity of the system. Additionally, the attacker can also use this access to gain control of the contract if the DEFAULT_ADMIN_ROLE is associated with privileged functions like upgrading the contract. Make these roles private or use some form of access control mechanism such as a roles library, which allows only authorized addresses to access these roles.

Proof of Concept

Attacker would first access the contract's state to read the MINTER_ROLE, PAUSER_ROLE and DEFAULT_ADMIN_ROLE by calling the corresponding public constant variable.

bytes32 minterRole = CashFactory.MINTER_ROLE;
bytes32 pauserRole = CashFactory.PAUSER_ROLE;
bytes32 defaultAdminRole = CashFactory.DEFAULT_ADMIN_ROLE;

Next, the attacker would use the acquired role bytes32 values to call the hasRole function of the cashImplementation contract, which is the ERC20 contract with the initializer disabled, to check if the attacker has the minter, pauser or admin role.

bool isMinter = cashImplementation.hasRole(minterRole, msg.sender);
bool isPauser = cashImplementation.hasRole(pauserRole, msg.sender);
bool isAdmin = cashImplementation.hasRole(defaultAdminRole, msg.sender);

If the attacker is able to acquire the minter role, they can mint new tokens and inflate the supply, which can have a significant financial impact on the token holders.

Attacker can create a simple contract that calls the hasRole function of the CashFactory contract and passes in the MINTER_ROLE constant. If the function returns true, the attacker can then call the mint function of the cashImplementation contract to mint new tokens.

pragma solidity 0.8.16;

contract Attacker {
    CashFactory factory;

    constructor(address _factory) public {
        factory = CashFactory(_factory);
    }

    function attack() public {
        if (factory.hasRole(CashFactory.MINTER_ROLE)) {
            factory.cashImplementation.mint(msg.sender, 100);
        }
    }
}

the attacker creates a contract that takes the address of the CashFactory contract as an argument in the constructor. The attack function is then called, which checks if the msg.sender(the address of the contract) has the MINTER_ROLE by calling the hasRole function of the CashFactory contract. If it returns true, the attacker can then call the mint function of the cashImplementation contract to mint new tokens. This would lead to the inflation of the token supply which can have a significant financial impact on the token holders.

Tools Used

Manual audit, Vs Code

Recommended Mitigation Steps

This vulnerability is to make the roles private and only accessible through a function call. For example, instead of setting the MINTER_ROLE as a public constant, it can be set as a private variable and a function can be added to check if a given address has the minter role.

pragma solidity 0.8.16;

contract CashFactory {
    // ...

    bytes32 private MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 private PAUSER_ROLE = keccak256("PAUSER_ROLE");
    bytes32 private DEFAULT_ADMIN_ROLE = bytes32(0);

    // ...

    function hasMinterRole(address _address) public view returns (bool) {
        return addressHasRole(_address, MINTER_ROLE);
    }

    function addressHasRole(address _address, bytes32 _role) internal view returns (bool) {
        // Code to check if the given address has the given role
    }

    // ...
}

Impact

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashFactory.sol#L49-L51

Cash public cashImplementation;
ProxyAdmin public cashProxyAdmin;
TokenProxy public cashProxy;

The public accessibility of these variables can allow an attacker to read their values and potentially manipulate them. For example, if the attacker were to change the value of the cashImplementation variable, they could potentially direct all future calls to a malicious contract that they control. Additionally, an attacker could change the values of the cashProxyAdmin and cashProxy variables to point to malicious contracts that they control, potentially allowing them to take over the administration of the contract and execute malicious actions. The attacker could potentially gain control over the entire contract by manipulating these variables. This could lead to financial loss and damage to the reputation of the contract.

Proof of Concept

A possible proof of concept for this bug could involve an attacker using a malicious contract to access the cashImplementation, cashProxyAdmin, and cashProxy variables and manipulate their values. For example, an attacker could use the following code snippet to access the cashImplementation variable and transfer all of the token's balance to their own address:

contract Attacker {
    CashFactory factory;

    constructor(address _factory) {
        factory = CashFactory(_factory);
    }

    function exploit() public {
        address attacker = msg.sender;
        Cash cash = Cash(factory.cashImplementation());
        cash.transfer(attacker, cash.balanceOf(factory.address));
    }
}

In this example, the attack deploys the malicious contract and passes the address of the CashFactory contract as an argument to its constructor. The exploit function is then called, which accesses the cashImplementation variable and transfers all of the token's balance to the attacker's address.

Alternatively, an attacker could manipulate the cashProxyAdmin and cashProxy variables to take control of the proxy contract and redirect the logic contract to a malicious implementation.

Tools Used

Manual audit, Vs Code.

Recommended Mitigation Steps

One step for this bug would be to change the visibility of the cashImplementation, cashProxyAdmin and cashProxy variables from public to internal or private. This would prevent external contracts and addresses from accessing and manipulating these variables.

contract CashFactory is IMulticall {
  // other code...

  internal Cash cashImplementation;
  internal ProxyAdmin cashProxyAdmin;
  internal TokenProxy cashProxy;

  // other code...
}

Another step would be to add access control to the variables, by creating a getter function that can only be called by authorized addresses.

Here's an example of how the code could be changed.

contract CashFactory is IMulticall {
    // other code...

    address internal guardian;
    Cash cashImplementation;
    ProxyAdmin cashProxyAdmin;
    TokenProxy cashProxy;

    constructor(address _guardian) {
        guardian = _guardian;
    }

    function getCashImplementation() public view returns(address) {
        require(msg.sender == guardian, "Unauthorized access");
        return cashImplementation;
    }

    function getCashProxyAdmin() public view returns(address) {
        require(msg.sender == guardian, "Unauthorized access");
        return cashProxyAdmin;
    }

    function getCashProxy() public view returns(address) {
        require(msg.sender == guardian, "Unauthorized access");
        return cashProxy;
    }
}

This way, I can certainly say only the guardian address can access the variables and retrieve the implementation contracts.

Impact

deployCash function is not implemented in the contract, so it is not possible to deploy the cash contract.

This bug means that the cash contract cannot be deployed and thus it will not be able to function as intended surely. This would prevent the contract from being used for its intended purpose, which could have a significant impact on the ONDO Finance project and its users. The contract may not be able to be used for its intended purpose, which could causefinancial losses` for the project and its users. Additionally, it could also prevent the contract from being upgraded in the future, which would make it difficult to fix bugs or add new features.

Proof of Concept

deployCash function is not implemented in the contract, which means that the cash contract cannot be deployed. This can have a significant impact on the functionality of the contract and prevent it from being used as intended.

To demonstrate this bug, we can create an instance of the CashFactory contract and call the deployCash function, which should deploy the cash contract. However, since the function is not implemented, it will throw an error or failure to execute.

// Create an instance of the CashFactory contract
CashFactory cashFactory = new CashFactory();

// Call the deployCash function
cashFactory.deployCash();

deployCash will fail and throw an error because the function is not implemented. This means that the cash contract cannot be deployed, and the contract cannot be used as intended.

Tools Used

Manual audit, Vs Code.

Recommended Mitigation Steps

deployCash function should be implemented in the contract. This function should deploy the cash contract, and set the cashImplementation, cashProxyAdmin and cashProxy variables to the deployed contract addresses. Here is an example of what the deployCash function could look like:

function deployCash(string memory name, string memory ticker) public {
    cashImplementation = new Cash();
    cashProxyAdmin = new ProxyAdmin();
    cashProxy = new TokenProxy(address(cashProxyAdmin), address(cashImplementation));

    cashImplementation.initialize(name, ticker);
}

In this example, the deployCash function creates new instances of the Cash, ProxyAdmin, and TokenProxy contracts and assigns them to the cashImplementation, cashProxyAdmin and cashProxy variables respectively. The cashImplementation contract is also initialized with the provided name and ticker.

It's also important to note that, deployCash function should be called by only authorized addresses, and the function should have a proper access control mechanism to prevent unauthorized access.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid