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

avaxAssignedHighWater is set wrongly in MinipoolManager.sol #193

Closed
code423n4 opened this issue Dec 28, 2022 · 2 comments
Closed

avaxAssignedHighWater is set wrongly in MinipoolManager.sol #193

code423n4 opened this issue Dec 28, 2022 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-566 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

code423n4 commented Dec 28, 2022

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.
avaxAssignedHighWater is set wrongly in MinipoolManager.sol at L373, particularly when the minipool was created via recreateMinipool().

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
According to the documentation of avaxAssignedHighWater at L26 of Staking.sol, ”staker.item.avaxAssignedHighWater = Highest amt of liquid staker funds assigned during a GGP rewards cycle“.
However, avaxAssignedHighWater is set wrongly in MinipoolManager.sol at L373 below:


if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
			staking.increaseAVAXAssignedHighWater(owner, avaxLiquidStakerAmt);
}

It is wrong because instead of replacing the avaxAssignedHighWater with a larger value of avaxAssigned, it adds avaxLiquidStakerAmt on top of the existing avaxAssignedHighWater , which is inconsistent with the definition.
In particular, when this minipool was created via recreateMinipool(), the new avaxLiquidStakerAmt is a compound value which contains the original avaxLiquidStakerAmt ALREADY. The original avaxLiquidStakerAmt will be added AGAIN in this case, which is wrong. Therefore, the safest way is to assign the larger avaxAssigned, based on the definition to be 100% correct.

Tools Used

Remix

Recommended Mitigation Steps

We can fix it with the following code, which will replace the existing avaxAssignedHighWater with the current larger value of avaxAssigned using function resetAVAXAssignedHighWater.

if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
			staking.resetAVAXAssignedHighWater(owner);
}

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 28, 2022
code423n4 added a commit that referenced this issue Dec 28, 2022
C4-Staff added a commit that referenced this issue Jan 6, 2023
@c4-judge
Copy link
Contributor

c4-judge commented Jan 9, 2023

GalloDaSballo marked the issue as duplicate of #566

@c4-judge
Copy link
Contributor

c4-judge commented Feb 8, 2023

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-566 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants