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.

Sign up for GitHub

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

Jump to bottom

function __abdicate() Remove Governor is Useless Because pendingGov Can Become Gov Again #76

Closed
code423n4 opened this issue Dec 3, 2021 · 2 comments
Closed

function __abdicate() Remove Governor is Useless Because pendingGov Can Become Gov Again #76

code423n4 opened this issue Dec 3, 2021 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly 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 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

  1. Alice want to Transfer Gov to Bob with setPendingGov() and now pendingGov = BobAddress
  2. Bob Accept Gov via acceptGov()
  3. Bob Tell Community He Want the Contract Become Trustless by Remove Governor via function __abdicate() and gov = address(0) now
  4. Later, Bob Call acceptGov() again because pendingGov = BobAddress and Bob Become Gov Again

Proof of Concept

https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L26-L30
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L33-L38
https://github.com/code-423n4/2021-11-streaming/blob/main/Streaming/src/Locke.sol#L45-L49

Tools Used

Manual Review

Recommended Mitigation Steps

In acceptGov(), Set pendingGov to Zero Address Once Transfer Gov.

address old = gov;
gov = pendingGov;
pendingGov = address(0);
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 3, 2021
code423n4 added a commit that referenced this issue Dec 3, 2021
@code423n4
Meta0xNull issue #76
e666bb0
@brockelmore brockelmore added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Dec 3, 2021
@brockelmore
Copy link
Collaborator

If there is a pendingGov you are correct, but since all of those functions are controlled by governance, it is trivial to reset pendingGov to 0 prior to abdicating, but it should be done in a single transaction.

@0xean
Copy link
Collaborator

0xean commented Jan 15, 2022

dupe of #132

@0xean 0xean closed this as completed Jan 15, 2022
@0xean 0xean added 3 (High Risk) Assets can be stolen/lost/compromised directly duplicate This issue or pull request already exists and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly 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 sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@0xean @brockelmore @code423n4