A Malicious DSS can easily bypass the slashing limits and slash all the Assets in a Vault

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/Vault.sol#L198
https://github.com/code-423n4/2024-07-karak/blob/main/src/NativeVault.sol#L311-L313

Vulnerability details

When a DSS requests slashing for a vault or operator, the SlasherLib.requestSlashing function calculates the amount to be slashed and stores it in earmarkedStakes. This exact earmarkedStakes amount is later used in the finalizeSlashing function to execute the slashAssets() operation.

The slashAssets function ensures that the amount to be slashed totalAssetsToSlash does not exceed the total assets currently available in the vault. If the totalAssetsToSlash is greater than the totalAssets(), then the slashing amount will be totalAssets().

    function slashAssets(uint256 totalAssetsToSlash, address slashingHandler) external onlyCore returns (uint256 transferAmount){
@>      transferAmount = Math.min(totalAssets(), totalAssetsToSlash);
        SafeTransferLib.safeApproveWithRetry(asset(), slashingHandler, transferAmount);
        ISlashingHandler(slashingHandler).handleSlashing(IERC20(asset()), transferAmount);
        emit Slashed(transferAmount);
    }

Issue

This opens the door for a malicious DSS to slash 100% of the assets available in the vault, regardless of whether the maxSlashablePercentageWad is set to 5%, 25%, or 50%. A DSS can deposit() a large amount into a vault and then call requestSlashing(), and then withdraw its funds before executing finalizeSlashing().

Proof of Concept

Let's consider a vault with 100 tokens/assets in it, registered with a DSS whose maxSlashablePercentageWad is 50%.

• The DSS finds the vault's operator unable to complete the task and eligible for slashing.
• The DSS deposits 105 tokens into that vault and quickly calls startRedeem(), so the total assets in that vault are now 205.
• After 8 days, the DSS calls requestSlashing() for that vault, which will set the earmarked stakes/total assets to slash to 102.5 tokens.
• Now, in the SLASHING_VETO_WINDOW, the DSS will finishRedeem() for its 105 tokens and get back its funds.
• Only 100 tokens are left in the vault.
• The SLASHING_VETO_WINDOW passes, and the DSS calls finalizeSlashing(), which executes slashAssets().
• The slashAssets function finds that total assets to slash is 102.5, but the total assets available are only 100, so it transfers all 100 tokens to the SlashingHandler.
• The vault got 100% slashed instead of the 50% (maxSlashablePercentageWad).

Impact

• Loss of Assets

Tools Used

Shaheen's Mindset

Recommended Mitigation Steps

Instead of setting the totalAssetsToSlash to the totalAssets(), when totalAssets() are less, re-calculate the totalAssetsToSlash or maybe revert the trx in that case, so the DSS can requestSlashing again.

Assessed type

Context

[c4-judge]

MiloTruck marked the issue as not a duplicate

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid

[MiloTruck]

The DSS is assumed to be non-malicious - operators will only register and deploy vaults for a DSS that it trusts.

The DSS deposits 105 tokens into that vault and quickly calls startRedeem(), so the total assets in that vault are now 205.
After 8 days, the DSS calls requestSlashing() for that vault, which will set the earmarked stakes/total assets to slash to 102.5 tokens.

You're assuming that a DSS will know 8 days in advance when it wants to slash an operator.

If the DSS wanted to slash 100% of assets, it could just call requestSlashing() with 100% or repeatedly call requestSlashing() and finalizeSlashing().

[c4-judge]

MiloTruck marked the issue as unsatisfactory:
Invalid

[TradMod]

Hey Judge Sahab @MiloTruck, Sir very respectfully, I think you have completely mis-understood this finding because all the points you mentioned above are incorrect.

The DSS is assumed to be non-malicious - operators will only register and deploy vaults for a DSS that it trusts.

The DSS is not a trusted party, it can behave maliciously, that's exactly why there are slashing percentage limits and VETO_COMITTEE exists. An operator will register to a DSS beliveing that the code is correct and it bound the DSS to behave correctly, and believing that if there is a malicious behaviors from a DSS, it will get blocked by the VETO_COMITTEE. The assumption that the DSS will always behave non-malicious is incorrect.

You're assuming that a DSS will know 8 days in advance when it wants to slash an operator.

Obviously a DSS will know when the operator is going to get slashed because DSS is the one that is going to slash operators. I think here, you are confusing the DSS Slashing with the Beacon chain slashing. Just to be clear, this vulnerability only exists in the Core/Vaults not in the Native Vault.
Even if we say that the DSS have no idea about when it will slash an operator, this vulnerability still exists. A DSS can deposit large funds at any time, maybe prior to the slashing event 1-2 months (when the operator will have single DSS, the DSS funds will be at no risk). He can deposit and quickly startRedeem() to make sure he can call finishRedeem any time after 9 days.

If the DSS wanted to slash 100% of assets, it could just call requestSlashing() with 100%

How is this is even possible, if the slashing limit is set to lets say 25%!?

or repeatedly call requestSlashing() and finalizeSlashing().

This is not possible as well, a DSS cannot randomly call requestSlashing() and finalizeSlashing(), there is a waiting period before he can requestSlashing again, also there is a VETO_COMIITEE to cancelSlashing all the randomly/unfairly slash requests.

The only was a DSS can slash 100% assets of the vault is by using this loop-hole, where VETO_COMIITEE will not gonna save users as well:

      transferAmount = Math.min(totalAssets(), totalAssetsToSlash);

Also In this particular case, the VETO_COMIITEE is not gonna be able to do anything as well, as it cannot recognize who deposits funds, a DSS can simply use another ethereum address to deposit funds, which will just gonna make him another depositer and when the slashing is happening fairly, there is no reason for the committee to interfere and cancel slashing. #76 (comment) raises a very fair point, that "finishRedeem and finalizeSlashing can be executed within the same transaction. This means that the so-called Slashing Veto Committee has no chance to detect the issue because, before the slash occurs, the DSS shares are still in the vault."

The mitigation I mentioned is on-point as well

Instead of setting the totalAssetsToSlash to the totalAssets(), when totalAssets() are less, re-calculate the totalAssetsToSlash

I request you Judge Sahab @MiloTruck to re-review this issue pls. And I would also like to request the sponsors to review this report @dewpe @karan-andalusia @devdks25. Thanks!

[MiloTruck]

Will duplicate this to #76 as they both have the same root cause.

Please do not tag the sponsors in an attempt to get your issues upgraded.

Also, I encourage you to be less aggressive and pushy in future escalations. It's probably unintentional, but you come off as extremely disrespectful in your comments.

[TradMod]

Extremely sorry, I highly respect you and other wardens, from whom I learn, I'll def gonna take care of that in the future 🙏

And Thanks for the re-review.