Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashFactory.sol#L96 https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L105 https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashKYCSenderFactory.sol#L105

Vulnerability details

Impact

The current ownership transfer process involves the current owner calling transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable. If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.

This could impact availability of protocol

Proof of Concept

https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashKYCSenderFactory.sol#L105

https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L105

https://github.com/code-423n4/2023-01-ondo/blob/83a45fa7cbc427a62ccf04d0e5cf9b0e780ec26c/contracts/cash/factory/CashFactory.sol#L96

The transferOwnership implementation in Openzeppelin is like this below

  /**
   * @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);
  }

Tools Used

Manual reading

Recommended Mitigation Steps

Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

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