code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L215 https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L183

Vulnerability details

Impact

When StaderStakePoolsManager is paused, deposits should be paused. Also, both StaderStakePoolsManager.validatorBatchDeposit and StaderStakePoolsManager.depositETHOverTargetWeight should be paused. But only StaderStakePoolsManager.validatorBatchDeposit has the whenNotPaused modifier. StaderStakePoolsManager.depositETHOverTargetWeight can still be called when StaderStakePoolsManager is paused.

Proof of Concept

When StaderStakePoolsManager is paused, StaderStakePoolsManager.validatorBatchDeposit cannot be called. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L183

    function validatorBatchDeposit(uint8 _poolId) external override nonReentrant whenNotPaused {

But StaderStakePoolsManager.depositETHOverTargetWeight is still able to be called. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L215

    function depositETHOverTargetWeight() external override nonReentrant {

Tools Used

Manual Review

Recommended Mitigation Steps

Add the whenNotPaused modifier on depositETHOverTargetWeight

    function depositETHOverTargetWeight() external override nonReentrant whenNotPaused {

Assessed type

Context

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #242

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

sces60107 commented 1 year ago

This issue is identified as a dup of https://github.com/code-423n4/2022-06-stader-findings/issues/242, which is currently classified as a quality assurance (QA) issue

The decision to downgrade is based on the comment provided by the sponsor in https://github.com/code-423n4/2022-06-stader-findings/issues/242

Only Staked funds related flows are required to be pausable by protocol design.

However, this issue is indeed related to staked funds. StaderStakePoolsManager.depositETHOverTargetWeight calls IStaderPoolBase(poolAddress).stakeUserETHToBeaconChain{value: validatorToDeposit * poolDepositSize}() to stake funds into the pool. Therefore, it should have the whenNotPaused modifier. https://github.com/code-423n4/2023-06-stader/blob/main/contracts/StaderStakePoolsManager.sol#L243

In conclusion, this issue should not be considered a dup of https://github.com/code-423n4/2022-06-stader-findings/issues/242. And the severity should be reconsidered

Picodes commented 1 year ago

@sanjay-staderlabs tagging you for visibility

sanjay-staderlabs commented 1 year ago

Hi @Picodes @sces60107 there is no need of pause in depositETHOverTargetWeight as this function is already safeguarded with a cooldown period of excessETHDepositCoolDown. Protocol can increase the cooldown to pause this function.

sces60107 commented 1 year ago

Hi @sanjay-staderlabs Technically, I agree that the protocol can definitely increase the cooldown period to pause depositETHOverTargetWeight. However, I don't think increasing cooldown period is a proper way to pause this function. Using two pause mechanisms simultaneously doesn't seem reasonable to me. The pause and unpause actions are split into two steps, which makes things complicated.

But if there is any good reason that the protocol want to pause only one specific function, I fully agree with your decision. And I fully respect @Picodes's final decision and will have no further dispute.