search

code-423n4 / 2022-05-cally-findings

2 stars 0 forks source link

SINGLE-STEP OWNERSHIP TRANSFER #271

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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