Single-step process for critical ownership transfer/renounce is risky #368
Labels
bug
Something isn't working
disagree with severity
Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments)
duplicate
This issue or pull request already exists
old-submission-method
QA (Quality Assurance)
Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax
sponsor acknowledged
Technically the issue is correct, but we're not going to resolve it for XYZ reasons
valid
Lines of code
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L100-L115
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L125-L139
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L150-L157
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L157-L168
Vulnerability details
Single-step process for critical ownership transfer/renounce is risky
Impact
The following functions of
HomeFiProxy.sol
, allow owners to interact with critical functions for adding new contracts and upgrade implementations, also to change the proxy owner:addNewContract
,upgradeMultipleImplementations
andchangeProxyAdminOwner
Given that
HomeFiProxy.sol
is derived from Ownable, the ownership management of this contract defaults to Ownable ’stransferOwnership()
andrenounceOwnership()
methods which are not overridden here.Same issue of transferring the ownership can be applied to the
proxyAdmin.transferOwnership(_newAdmin)
in one step.Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes
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() or onlyOwnerOrAssetManager() 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.
Github Permalinks
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L100-L115
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L125-L139
https://github.com/code-423n4/2022-08-rigor/blob/e35f5f61be9ff4b8dc5153e313419ac42964d1fd/contracts/HomeFiProxy.sol#L150-L157
Other instances of this issue
https://github.com/code-423n4/2022-08-rigor/blob/f2498c86dbd0e265f82ec76d9ec576442e896a87/contracts/HomeFi.sol#L157-L168
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/v3-core/blob/main/audits/tob/audit.pdf
Recommended steps
Recommend overriding the inherited methods to null functions and use separate functions for a two-step address 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 time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.
The text was updated successfully, but these errors were encountered: