Given that Factory contract is derived from Ownable, the ownership management of this contract defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky for Factory contract which controls the entire protocol because it is irrecoverable from any mistakes.
Interestingly, a 2-step change is used for changing of Basket’s Publisher and LicenseFee but it is more critical for the Factory contract.
Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.
Override the inherited methods to null functions and use separate functions for a two-step address change: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
Also, consider adding a timelock delay for such sensitive actions (similar to what’s done in Basket functions). And at a minimum, use a multisig (with mutually independent and trustworthy owners) and not an EOA.
Handle
0xRajeev
Vulnerability details
Impact
Given that Factory contract is derived from Ownable, the ownership management of this contract defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky for Factory contract which controls the entire protocol because it is irrecoverable from any mistakes.
Interestingly, a 2-step change is used for changing of Basket’s Publisher and LicenseFee but it is more critical for the Factory contract.
Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately. When noticed, due to a failing onlyOwner() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.
Proof of Concept
See similar High Risk severity finding from Trail-of-Bits Audit of Hermez: https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf
See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/uniswap-v3-core/blob/main/audits/tob/audit.pdf
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L13
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Factory.sol#L39-L55
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/01f2ff1ba1220d7d3a0642014d30543bfcbb41ee/contracts/access/Ownable.sol#L53-L64
Tools Used
Manual Analysis
Recommended Mitigation Steps
Override the inherited methods to null functions and use separate functions for a two-step address change: 1) Approve a new address as a pendingOwner 2) A transaction from the pendingOwner address claims the pending ownership change. This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.
Also, consider adding a timelock delay for such sensitive actions (similar to what’s done in Basket functions). And at a minimum, use a multisig (with mutually independent and trustworthy owners) and not an EOA.