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

The state of MinipoolStatus.Error should only be transitioned to from MinipoolStatus.Launched #829

Closed
code423n4 opened this issue Jan 3, 2023 · 4 comments
Labels
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 duplicate-471 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor duplicate Sponsor deemed duplicate

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L480
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152

Vulnerability details

Impact

If the system is allowed to transition into MinipoolStatus.Error state, after recordStakingStart() has been called, whatever staking reward that might have been accumulated will not be recoverable to the skater.

Proof of Concept

The comment for recordStakingError() says:
A staking error occured while registering the node as a validator

However, requireValidStateTransition() also allows the transition to MinipoolStatus.Error from MinipoolStatus.Staking after recordStakingStart() is called.

Once in Error state, the only next transitionable states are Finished or Prelaunch. In both cases, there are no ways for the recovery of any potential staking reward that might have been accumulated during the duration.

Tools Used

manual

Recommended Mitigation Steps

Disable the transition from MinipoolStatus.Staking to MinipoolStatus.Error.

} else if (currentStatus == MinipoolStatus.Staking) {
    isValid = (to == MinipoolStatus.Withdrawable;
}
@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 Jan 3, 2023
code423n4 added a commit that referenced this issue Jan 3, 2023
C4-Staff added a commit that referenced this issue Jan 6, 2023
@emersoncloud emersoncloud added needs-discussion for sponsor to discuss with their team disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) and removed needs-discussion for sponsor to discuss with their team labels Jan 15, 2023
@emersoncloud
Copy link

Duplicate of #471

@emersoncloud emersoncloud marked this as a duplicate of #471 Jan 20, 2023
@emersoncloud emersoncloud added sponsor duplicate Sponsor deemed duplicate sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Jan 20, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Feb 3, 2023

GalloDaSballo marked the issue as duplicate of #471

@GalloDaSballo
Copy link

FSM Mistake is correct, but Impact is not

Awarding half

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Feb 3, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Feb 3, 2023

GalloDaSballo marked the issue as partial-50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 duplicate-471 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sponsor duplicate Sponsor deemed duplicate
Projects
None yet
Development

No branches or pull requests

4 participants