eeyore - Premature collateralization check in the BaseStakingVault.initiateWithdraw() function can leave accounts undercollateralized

[sherlock-admin4]

eeyore

Medium

Premature collateralization check in the BaseStakingVault.initiateWithdraw() function can leave accounts undercollateralized

Summary

The collateralization check is currently performed before the user action that impacts the account's collateralization.

Vulnerability Detail

The initiateWithdraw() function can affect the solvency of the account. During this process, tokens may be unwrapped and new tokens pushed into the withdrawal queue, altering the underlying tokens for which collateralization was initially checked. This can result in a different collateralization level than initially assessed.

Additionally, this contradicts how Notional core contracts perform such checks, where they are always conducted as the final step in any user interaction.

Impact

The account may become undercollateralized or insolvent following the user action.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/BaseStakingVault.sol#L250-L255

Tool used

Manual Review

Recommendation

Perform the account collateralization check after the _initiateWithdraw() function call:

function initiateWithdraw() external {
+       _initiateWithdraw({account: msg.sender, isForced: false});

        (VaultAccountHealthFactors memory health, /* */, /* */) = NOTIONAL.getVaultAccountHealthFactors(
            msg.sender, address(this)
        );
        VaultConfig memory config = NOTIONAL.getVaultConfig(address(this));
        // Require that the account is collateralized
        require(config.minCollateralRatio <= health.collateralRatio, "Insufficient Collateral");

-       _initiateWithdraw({account: msg.sender, isForced: false});
    }
}

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Lacks proof to substantiate the bug on an intended design

Hash01011122 commented:

Invalid, If this were to occur it would be admin error to add any other underlying asset

[T-Woodward]

I think this is a valid finding. Medium is a reasonable severity imo.

@jeffywu this reinforces the need to value withdraw requests as if they were still the staked asset except in the very strict case where we know exactly what we will get upon unstaking and exactly when we will get it

[0xklapouchy]

@mystery0x

To simplify the issue described, let's illustrate it using the PendlePTKelpVault.

Before initiating a withdrawal, the vault shares' value is calculated based on the rsETH/WETH price. After a withdrawal request, the calculation is based on the stETH/WETH price.

In a situation where, for some reason, the stETH price drops compared to rsETH, the user's shares' value will also drop after initiating the withdrawal request, leading to the position becoming unhealthy for both the user and the protocol.

In such a case, it is better to block the initiation of the withdrawal. The better operation for the user or the Notional protocol in such cases is to close the position via _executeInstantRedemption(), which will redeem rsETH to WETH via TRADING_MODULE at a better price.

[0xklapouchy]

Escalate.

This is a valid medium issue.

[sherlock-admin3]

Escalate.

This is a valid medium issue.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page,
in the Sherlock webapp.

[xiaoming9090]

Escalate.

Escalating this issue on behalf of the submitter. Please review the above comments. Thanks.

[sherlock-admin3]

Escalate.

Escalating this issue on behalf of the submitter. Please review the above comments. Thanks.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

Agree with the escalation, planning to accept it and validate with medium severity. @mystery0x are there additional duplicates?

[iso0x]

I believe it is unique.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/notional-finance/leveraged-vaults/pull/95/files

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.