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

SavETHVault / StakingFundsVault: maxStakingAmountPerValidator may not be reachable #140

Closed
code423n4 opened this issue Nov 17, 2022 · 7 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-422 satisfactory satisfies C4 submission criteria; eligible for awards 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

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/a0558ed7b12e1ace1fe5c07970c7fc07eb00eebd/contracts/liquid-staking/ETHPoolLPFactory.sol#L111
https://github.com/code-423n4/2022-11-stakehouse/blob/23c3cf65975cada7fd2255a141b359a6b31c2f9c/contracts/syndicate/Syndicate.sol#L215

Vulnerability details

Impact

In ETHPoolLPFactory._depositETHForStaking, a minimum deposit amount (set to 0.001 ether) is required:

require(_amount >= MIN_STAKING_AMOUNT, "Min amount not reached");

On the other hand, the maximum amount is also restricted such that maxStakingAmountPerValidator will not be exceeded:

require(lpToken.totalSupply() + _amount <= maxStakingAmountPerValidator, "Amount exceeds the staking limit for the validator");

This combination of checks can lead to situations where maxStakingAmountPerValidator is never reachable, namely when lpToken.totalSupply() > maxStakingAmountPerValidator - MIN_STAKING_AMOUNT. Every deposit that is larger than MIN_STAKING_AMOUNT will revert because it would exceed the maximum staking amount and every deposit that is smaller reverts because this is the enforced minimum.

Note that the same problem also can occur in Syndicate.stake where a minimum stake amount of 1 gwei is enforced, but the total staked amount cannot exceed 12 ether.

Proof Of Concept

Let's say that a StakingFundsVault for a given BLS public key has already 4 - 0.001 ether and 1 wei. When a user now wants to deposit < 0.001 ether, the call reverts because the minimum amount is not reached. For more than 0.001 ether, the call reverts because this would exceed the staking limit.

Recommended Mitigation Steps

Do not allow deposits that bring the total supply into the range [maxStakingAmountPerValidator - MIN_STAKING_AMOUNT, maxStakingAmountPerValidator - 1].

@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 Nov 17, 2022
code423n4 added a commit that referenced this issue Nov 17, 2022
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Nov 20, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as primary issue

@c4-judge c4-judge added G (Gas Optimization) downgraded by judge Judge downgraded the risk level of this issue and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Nov 20, 2022
@c4-judge
Copy link
Contributor

dmvt changed the severity to G (Gas Optimization)

@dmvt
Copy link

dmvt commented Nov 20, 2022

Note, this should be medium, not Gas. Accidental button click on my end.

@Simon-Busch Simon-Busch added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed downgraded by judge Judge downgraded the risk level of this issue G (Gas Optimization) labels Nov 21, 2022
@Simon-Busch
Copy link
Contributor

@dmvt Fixed and revert as per your request

@c4-sponsor
Copy link

vince0656 marked the issue as sponsor confirmed

@c4-judge
Copy link
Contributor

dmvt marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 30, 2022
@C4-Staff
Copy link
Contributor

JeeberC4 marked the issue as duplicate of #422

@C4-Staff C4-Staff added duplicate-422 and removed primary issue Highest quality submission among a set of duplicates labels Dec 21, 2022
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-422 satisfactory satisfies C4 submission criteria; eligible for awards 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

6 participants