code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashFactory.sol#L86-L94 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashKYCSenderFactory.sol#L98-L105 https://github.com/code-423n4/2023-01-ondo/blob/f3426e5b6b4561e09460b2e6471eb694efdd6c70/contracts/cash/factory/CashKYCSenderReceiverFactory.sol#L98-L105

Vulnerability details

Impact

The guardian in CashFactory, CashKYCSenderFactory and CashKYCSenderReceiverFactory is the owner of the cashProxyAdmin() contracts and holds the role DEFAULT_ADMIN_ROLE and PAUSER_ROLE. If the guardian address has to be quickly changed e.g. if a multisig wallet has a vulnerability or if an EOA is deemed unsafe the owner of all proxyAdmins has to be quickly changed and the guardians roles in Cash contracts has to be revoked and given to a new guardian.

Proof of Concept

In the case of an emergency where the guardian has to be changed the cashProxyAdmins and every Cash contract all have to be individually upgraded. In such a scenario it is important to be able to quickly change the access control of each contract quickly.

Consider what an attacker could do if they have control of the guardian: they could use flashbots to change all access control and ownership in a single block without Ondo having a chance to react. If the guardian is completely compromised there is no way of protecting the protocol. Adding this functionality does not increase the risk since an attack could achieve the same thing anyways

But in a case where a vulnerability is detected and guardian has not yet been compromised Ondo has to be able to quickly gain control of all contracts.

Tools Used

Manual Review

Recommended Mitigation Steps

Create a function in the factory contract that takes in a list of addresses and delegatecalls those addresses to change ownership and roles in cashProxyAdmin and the Cash contracts. guardian should also be changed from immutable to private and it should also be upgraded in such a scenario so that the same factory can be used even if the guardian has been changed.

A separate contract can be deployed that manages these concerns. The contract would be able to change guardians in all the factories and update the ownership and access control in all proxyAdmins and cash contracts.