Single-step process for critical ownership transfer is risky
Impact
The following contracts and functions, allow owners to interact with core functions such as:
Functions from ArtGobblers.sol:
upgradeRandProvider() -> to update rand provider, what can be important in some future
Functions from Owned.sol:
setOwner() to set new Owners
Functions from GobblerReserve.sol:
withdraw() -> To withdraw all gobblers NFT from the reserve
Such critical address transfer in Owned.sol 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() 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.
Also neither the constructor neither setOwner check for 0 address, what makes this more feasible to happen
Recommend use functions for a two-step address change:
Approve a new address as a pendingOwner
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 time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.
Lines of code
https://github.com/code-423n4/2022-09-artgobblers/blob/fb54f92ffcb0c13e72c84cde24c138866d9988e8/src/ArtGobblers.sol#L560 https://github.com/transmissions11/solmate/blob/34d20fc027fe8d50da71428687024a29dc01748b/src/auth/Owned.sol#L39 https://github.com/code-423n4/2022-09-artgobblers/blob/fb54f92ffcb0c13e72c84cde24c138866d9988e8/src/utils/GobblerReserve.sol#L34
Vulnerability details
Single-step process for critical ownership transfer is risky
Impact
The following contracts and functions, allow owners to interact with core functions such as: Functions from
ArtGobblers.sol
:upgradeRandProvider()
-> to update rand provider, what can be important in some futureFunctions from
Owned.sol
:setOwner()
to set new OwnersFunctions from
GobblerReserve.sol
:withdraw()
-> To withdraw all gobblers NFT from the reserveSuch critical address transfer in
Owned.sol
one-step is very risky because it is irrecoverable from any mistakesScenario: 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.Also neither the constructor neither
setOwner
check for 0 address, what makes this more feasible to happenGithub Permalinks
https://github.com/code-423n4/2022-09-artgobblers/blob/fb54f92ffcb0c13e72c84cde24c138866d9988e8/src/ArtGobblers.sol#L560
https://github.com/transmissions11/solmate/blob/34d20fc027fe8d50da71428687024a29dc01748b/src/auth/Owned.sol#L39
https://github.com/code-423n4/2022-09-artgobblers/blob/fb54f92ffcb0c13e72c84cde24c138866d9988e8/src/utils/GobblerReserve.sol#L34
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 use functions for a two-step address change:
pendingOwner
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 time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.