Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol#L51 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol#L56 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol#L41 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/lending/tokens/cToken/CTokenInterfacesModified.sol#L46

Vulnerability details

Impact

  /**
   * @notice Contract which oversees inter-cToken operations
   */
  ComptrollerInterface public comptroller;

  /**
   * @notice Model which tells what the current interest rate should be
   */
  InterestRateModel public interestRateModel;

  /**
   * @notice Administrator for this contract
   */
  address payable public admin;

  /**
   * @notice Pending administrator for this contract
   */
  address payable public pendingAdmin;

The code defines the public variables comptroller, interestRateModel, admin, and pendingAdmin without any structure in place to restrict the admin or pendingAdmin from changing these variables to malicious contracts, which could lead to a loss of funds for users.

The vulnerability allows the admin or pendingAdmin to change the ComptrollerInterface and InterestRateModel contracts to malicious ones without any restrictions. The ComptrollerInterface contract is responsible for inter-cToken operations, and the InterestRateModel contract is responsible for determining the current interest rate. Changing these contracts to malicious ones can have a significant impact on the system's functionality and the users' funds.

Malicious admin or pendingAdmin could change the ComptrollerInterface contract to one that allows them to transfer all the users' funds to their own account. Malicious admin or pendingAdmin could also change the InterestRateModel contract to one that increases the interest rate drastically, leading to an unsustainable increase in borrows and a decrease in reserves. This could result in the collapse of the system, causing a loss of funds for users.

Additionally, if the new ComptrollerInterface contract or InterestRateModel contract has a vulnerability, attackers can exploit it and steal the users' funds. Therefore, it is essential to have a mechanism in place that prevents the admin or pendingAdmin from changing the ComptrollerInterface and InterestRateModel contracts to malicious ones, to protect the users' funds and the overall integrity of the system.

Proof of Concept

Here is how clearly an attacker can change the ComptrollerInterface and InterestRateModel contracts to malicious ones and steal the users' funds.

contract CTokenStorage {
    address payable public admin;
    address payable public pendingAdmin;
    ComptrollerInterface public comptroller;
    InterestRateModel public interestRateModel;
    ...
    function setComptroller(address _comptroller) public {
        require(msg.sender == admin || msg.sender == pendingAdmin, "Only admin or pendingAdmin can call this function");
        comptroller = ComptrollerInterface(_comptroller);
    }
    function setInterestRateModel(address _interestRateModel) public {
        require(msg.sender == admin || msg.sender == pendingAdmin, "Only admin or pendingAdmin can call this function");
        interestRateModel = InterestRateModel(_interestRateModel);
    }
    ...
}

setComptroller() and setInterestRateModel() functions allow the admin or pendingAdmin to change the ComptrollerInterface and InterestRateModel contracts. However, there is no mechanism in place to prevent them from setting these contracts to malicious ones.

An attacker can create a malicious ComptrollerInterface contract that allows them to transfer all the users' funds to their own accounts. The attacker can then use the setComptroller() function to set this contract as the new ComptrollerInterface and steal the users' funds.

Similarly, an attacker can create a malicious InterestRateModel contract that increases the interest rate drastically, leading to an unsustainable increase in borrows and a decrease in reserves. This could result in the collapse of the system, causing a loss of funds for users. The attacker can then use the setInterestRateModel() function to set this contract as the new InterestRateModel.

Tools Used

Manual audit, Vs Code

Recommended Mitigation Steps

Add a whitelist of approved contracts that the admin or pendingAdmin can set as the ComptrollerInterface or InterestRateModel. This whitelist can be set by a higher authority or a group of trusted individuals to ensure that only approved contracts can be set.

    address[] public approvedComptrollerInterfaces;
    address[] public approvedInterestRateModels;

    function setComptroller(address _comptroller) public {
        require(msg.sender == admin || msg.sender == pendingAdmin, "Only admin or pendingAdmin can call this function");
        require(addressIsApproved(_comptroller, approvedComptrollerInterfaces), "Contract is not on the approved ComptrollerInterface whitelist");
        comptroller = ComptrollerInterface(_comptroller);
    }
    function setInterestRateModel(address _interestRateModel) public {
        require(msg.sender == admin || msg.sender == pendingAdmin, "Only admin or pendingAdmin can call this function");
        require(addressIsApproved(_interestRateModel, approvedInterestRateModels), "Contract is not on the approved InterestRateModel whitelist");
        interestRateModel = InterestRateModel(_interestRateModel);
    }
    function addressIsApproved(address _contract, address[] memory _whitelist) internal view returns(bool) {
for (uint i = 0; i < _whitelist.length; i++) {
if (_whitelist[i] == _contract) {
return true;
}
}
return false;
}

approvedComptrollerInterfaces and approvedInterestRateModels variables are whitelists of approved contracts that the admin or pendingAdmin can set as the ComptrollerInterface or InterestRateModel. The addressIsApproved() function checks if the contract being set is on the appropriate whitelist, and only allows the setting of the contract if it is.

The whitelist should be managed by a higher authority or a group of trusted individuals to ensure that only approved contracts can be set.

Additionally, it would be ideal if the whitelist is updated by a smart contract rather than a manual process. This would ensure that the whitelist is always up-to-date and secure.

Finally, an emergency function that can pause/freeze the contract should be added, this function should be protected by a safety mechanism such as a time lock or a multi-signature, which will ensure that the contract can only be paused under certain conditions or by a specific group of people.

code-423n4 / 2023-01-ondo-findings