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

[High-4] The slashing amount that is calculated is likely to be too high #490

Closed
code423n4 opened this issue Jan 3, 2023 · 2 comments
Closed
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-493 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L673
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L34
https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L30

Vulnerability details

Impact

In the current implementation of the MinipoolManager, when a minipool node gets slahed because it didn't had a sufficient uptime during validation, the slashing amount calculation is based on the full node validation duration while it should only be based on the time interval of the cycles, meaning 14 days as per the Notion documentation:

Flow 

1. Liquid stakers deposit AVAX in exchange for ggAVAX
2. When there are sufficient funds deposited and a minipool to match, AVAX is withdrawn by the minipool for staking. 
3. After the staking period, AVAX used for staking and Avalanche rewards are deposited back to the ggAVAX contract. 
4. Rewards are reflected in the exchange rate of ggAVAX → AVAX and are streamed over a 14 day period. 

In the slash() function, it seems like the duration that is taken to estimate the slashing amount based on the expected reward rate of the node is the one concerning the full validation period of the node. This is one comment that seems to confirm this concern in the MinipoolManager:

`minipool.item<index>.duration = requested validation duration in seconds (performed as 14 day cycles)`

As well as this comment:

/// @param duration Requested validation period in seconds

In the current state, that means that if the operator is staking for a duration of 1 year or 14 days, in the case of slashing, they won't be slashed the same amount.
It is an issue because rather than incentivizing minipool operators to stake for a long period, it is rather penalizing them in the case their node does not maintain a sufficient uptime.
Also, this slashing amount gets exponentially higher as the validation duration is bigger, because it is going to be called more times (a bigger validation duration can fit more streamed 14 days period), with a more painful slash (calculated from the whole duration rather than the constant 14 days).

Proof of Concept

  1. Bob registers a minipool for 1 year
  2. Bob does not maintain a sufficient node uptime for the first reward cycle
  3. Bob expect to be slashed for this 14 days period, but instead is slashed for the whole year, which is about 26 times too much

Tools Used

Manual Inspection

Recommended Mitigation Steps

There could be a constant registered in the ProtocolDAO that store this 14 days constant, and the duration used to calculate the amount of ggp to slash should be always using this constant.
The RewardsEligibilityMinSeconds seems to be related, albeit doesn't seem to fully match the purpose of this validation cycle interval.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly 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
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as duplicate of #493

@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-493 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants