SINGLE-STEP OWNERSHIP TRANSFER

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-cally/blob/1849f9ee12434038aa80753266ce6a2f2b082c59/contracts/src/Cally.sol#L124
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/57725120581e27ec469e1c7e497a4008aafff818/contracts/access/Ownable.sol#L62

Vulnerability details

SINGLE-STEP OWNERSHIP TRANSFER

The current ownership transfer process involves the current owner calling transferOwnership(), from the OpenZeppelin Ownable contract. This function checks the new owner is not the zero address and proceeds to write the new owner’s address into the owner state variable. If the current owner writes the wrong address (e.g a typo) and the nominated EOA account is not a valid account, the functions with the onlyOwner modifier will not be able to be called anymore. In particular the function withdrawProtocolFees(), meaning all the protocol fees will be locked forever in the contract.

Impact

Medium

Proof Of Concept

Cally:124: function withdrawProtocolFees() external onlyOwner returns (uint256 amount)

Tools Used

Manual Analysis

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.

[outdoteth]

This requires the owner to actively make a mistake when upgrading to a new owner which is very unlikely since it will like only happen at most once every few months via multisig.

[HardlyDifficult]

Lowing to 1 (Low) and grouping with the warden's QA report #268