search

code-423n4 / 2023-01-drips-findings

0 stars 2 forks source link

Lack of double step transfer in admin modification in a upgradeable contract is dangerous #299

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/a271fcc95ed1f2953bfef2345c86c0035a1dffbf/src/Managed.sol#L84-L86 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/Managed.sol#L4

Vulnerability details

Lack of double step transfer in admin modification in a upgradeable contract is dangerous

Summary

Double step transfer of admin / ownership should be a must in upgradeable contracts

Vulnerability Detail

Admin is changed with changeAdmin that calls _changeAdmin, this is a 1 step transfer provided by ERC1967Proxy. If wrongly set to a malicious actor, contract can call upgradeTo or upgradeToAndCall and destroy the contract. At best, wrong user address provided won't notice being admin of this contract and just all the upgrade and onlyAdmin functions related to pause will be unusable.

Impact

Ownership can be lost, contract can be destroyed calling upgradeTo or upgradeToAndCall from UUPSUpgradeable

Code Snippet

Tool used

Manual Review

Recommendation

Use a 2 steps transfer of ownership for changeAdmin where a pendingAdmin is set and that one is the one who has to accept the change before switching the role by calling _changeAdmin. In case the address is wrong, it can be called setPendingAdmin and set a correct address

GalloDaSballo commented 1 year ago

Non Critical at most, closing as Overly Inflated.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity