code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/external/openzeppelin/contracts/proxy/ProxyAdmin.sol#L63 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/external/openzeppelin/contracts/proxy/TransparentUpgradeableProxy.sol#L89 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/external/openzeppelin/contracts/access/Ownable.sol#L65 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/external/openzeppelin/contracts/access/Ownable.sol#L57

Vulnerability details

Impact

In the cash part of the code, the contract TokenProxy in Proxy.sol serves as a generic proxy for all tokens deployed using factory contracts.

The factory contracts deploy a ProxyAdmin contract and a TokenProxy, alongside the respective token implementation.

Both changeAdmin(), changeProxyAdmin() or transferOwnership() perform this ownership transition in one single step. Also the renounceOwnership() action can change the ownership of the AdminProxy contract to address(0) without any option of revert.

A malicious admin or an error in the address when calling any of these functions can prevent admin activities on the token and admin contracts forever.

Proof of Concept

Ownable transfer of ownership:

  /**
   * @dev Leaves the contract without owner. It will not be possible to call
   * `onlyOwner` functions anymore. Can only be called by the current owner.
   *
   * NOTE: Renouncing ownership will leave the contract without an owner,
   * thereby removing any functionality that is only available to the owner.
   */
  function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
  }

  /**
   * @dev Transfers ownership of the contract to a new account (`newOwner`).
   * Can only be called by the current owner.
   */
  function transferOwnership(address newOwner) public virtual onlyOwner {
    require(newOwner != address(0), "Ownable: new owner is the zero address");
    _transferOwnership(newOwner);
  }

  /**
   * @dev Transfers ownership of the contract to a new account (`newOwner`).
   * Internal function without access restriction.
   */
  function _transferOwnership(address newOwner) internal virtual {
    address oldOwner = _owner;
    _owner = newOwner;
    emit OwnershipTransferred(oldOwner, newOwner);
  }

Or, TransparentUpgradeableProxy transfer of admin role:

  /**
   * @dev Changes the admin of the proxy.
   *
   * Emits an {AdminChanged} event.
   *
   * NOTE: Only the admin can call this function. See {ProxyAdmin-changeProxyAdmin}.
   */
  function changeAdmin(address newAdmin) external virtual ifAdmin {
    _changeAdmin(newAdmin);
  }

  /**
   * @dev Stores a new address in the EIP1967 admin slot.
   */
  function _setAdmin(address newAdmin) private {
    require(newAdmin != address(0), "ERC1967: new admin is the zero address");
    StorageSlot.getAddressSlot(_ADMIN_SLOT).value = newAdmin;
  }

  /**
   * @dev Changes the admin of the proxy.
   *
   * Emits an {AdminChanged} event.
   */
  function _changeAdmin(address newAdmin) internal {
    emit AdminChanged(_getAdmin(), newAdmin);
    _setAdmin(newAdmin);
  }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Recommend not using the standard Ownable or TransparentUpgradeableProxy contracts directly as they are, and change the transfer of ownership/admin privileges to be a two step process, like it is implemented in other contracts, like contracts/lending/tokens/cCash/CTokenCash.sol, where you have a _setPendingAdmin() and a _acceptAdmin() calls to start and finalize the process.

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Out of scope

c4-sponsor commented 1 year ago

tom2o17 marked the issue as sponsor acknowledged

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