Unchecked call to `transferOwnership()` function in CashFactory contract

[code423n4]

Lines of code

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

Vulnerability details

Summary

The deployCash() function in the CashFactory contract calls the transferOwnership() function on the cashProxyAdmin contract without checking if the call succeeded or not. If the call to transferOwnership() fails, the assert(cashProxyAdmin.owner() == guardian); will also fail and the contract will be halted.

Impact

If the call to transferOwnership() fails, the ownership of the cashProxyAdmin contract will not be transferred to the intended guardian address, leaving the contract open to potential malicious actors.

Proof of Concept

https://github.com/code-423n4/2023-01-ondo/blob/main/contracts/cash/factory/CashFactory.sol

Tools Used

Manual.

Recommended Mitigation Steps

Add a check for the return value of the transferOwnership() call and add a require statement to ensure that the call to transferOwnership() succeeded, or revert the transaction if it fails.

require(cashProxyAdmin.transferOwnership(guardian), "Transfer of ownership failed");

or

if(!cashProxyAdmin.transferOwnership(guardian)) revert("Transfer of ownership failed")

Also, it is a good practice to add a require statement to ensure that the guardian address passed to the contract is not the zero address.

require(_guardian != address(0),"Guardian address should not be zero address")

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid