Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reassignGlobalAdmin() Have No Transfer Ownership Pattern #112

Open
code423n4 opened this issue Nov 29, 2021 · 1 comment
Open

reassignGlobalAdmin() Have No Transfer Ownership Pattern #112

code423n4 opened this issue Nov 29, 2021 · 1 comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Handle

Meta0xNull

Vulnerability details

Impact

The current ownership transfer process involves the current TIMELOCK_ROLE calling reassignGlobalAdmin(). If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the TIMELOCK_ROLE modifier.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L63-L77

Tools Used

Manual Review

Recommended Mitigation Steps

Consider implementing a two step process where the TIMELOCK_ROLE nominates an account and the nominated account needs to call an accept_TIMELOCK_ROLE() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Nov 29, 2021
code423n4 added a commit that referenced this issue Nov 29, 2021
@0xScotch 0xScotch added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Dec 8, 2021
@GalloDaSballo
Copy link
Collaborator

Personally I don't believe the "2 step governance change" to be a low severity finding, however the majority of contest rates it as such so we'll allow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants