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

Malicios Operator can utilize a malicios DSS and an overleveraged vault to frontrun an honest DSS's slashing request to protect its funds #27

Open
c4-bot-10 opened this issue Jul 30, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-b insufficient quality report This report is not of sufficient quality Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_06_group AI based duplicate group recommendation

Comments

@c4-bot-10
Copy link
Contributor

c4-bot-10 commented Jul 30, 2024

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/main/src/Core.sol#L262
https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/SlasherLib.sol#L174
https://github.com/code-423n4/2024-07-karak/blob/main/src/SlashingHandler.sol#L57

Vulnerability details

Impact

By design, the protocol allows vaults to be overleveraged, meaning they can be staked to more than 1 DSS. Also, if an operator is found to be malicios a DSS can slash the funds of it's vault. Furthermore, anyone can register as a DSS.

This means that a malicios operator can utilize a custom DSS with 100% slashing to stake his funds and then also stake his funds to an honest DSS. The operator would attempt a malicios operation on the DSS, which would result in a slashing event being submitted by the honest DSS. The operator can then monitor the mempool and frontrun the slashing event transaction by submitting a slashing event from his own DSS for 100% as well.

The default implementation of the slashing handler in the protocol burns the slashed funds, however, it is common practice in these types of systems, to distribute the slashed funds to honest operators, by for example sending them first to a ledger on the DSS side. Other slashing handlers are expected to be made.

The attacker would select an asset with a slashing handler that would allow him to return the funds slashed by his own DSS. This way the attacker can refund his own funds. This is assuming that the veto committee does not recongnize that the operator is working with his own DSS, which can be very difficult to do, if the DSS is designed to appear honest, considering another honest DSS would be slashing this operator as well, it will be easy to mistake the malicios slashing for an honest one.

Proof of Concept

The protocol allows anyone to register as a DSS:

https://github.com/code-423n4/2024-07-karak/blob/main/src/Core.sol#L262

    /// @notice Allows the caller to register as a DSS and set the max slashable percentage
    /// @dev If you want to have 0% slashing, set `maxSlashablePercentageWad` to `1` since 0 is used as a default flag for not set
    /// If a operator is set, it will have a non-zero slashable percentage. Hence we can just check for that to see if a DSS is registered
    /// @param maxSlashablePercentageWad The max slashable percent, in the form of wad, that the DSS can slash per cooldown period
    function registerDSS(uint256 maxSlashablePercentageWad) external {
        IDSS dss = IDSS(msg.sender);
        if (!address(dss).isSmartContract()) revert NotSmartContract();
        _self().setDSSMaxSlashablePercentageWad(dss, maxSlashablePercentageWad);
        emit DSSRegistered(msg.sender, maxSlashablePercentageWad);
    }

The maxSlashablePercentageWad can be up to 100%, meaning a DSS can slash up to 100% of a staked operator's funds:

https://github.com/code-423n4/2024-07-karak/blob/main/src/entities/SlasherLib.sol#L174

    function setDSSMaxSlashablePercentageWad(
        CoreLib.Storage storage self,
        IDSS dss,
        uint256 dssMaxSlashablePercentageWad
    ) public {
        uint256 currentSlashablePercentageWad = self.dssMaxSlashablePercentageWad[dss];
        if (currentSlashablePercentageWad != 0) revert DSSAlreadyRegistered();
        if (dssMaxSlashablePercentageWad == 0) revert ZeroSlashPercentageWad();
        if (dssMaxSlashablePercentageWad > Constants.MAX_SLASHING_PERCENT_WAD) revert MaxSlashPercentageWadBreached();
        self.dssMaxSlashablePercentageWad[dss] = dssMaxSlashablePercentageWad;
    }

With Constants.MAX_SLASHING_PERCENT_WAD = 100%.

The default SlashingHandler implementation is to burn the slashed funds, however it is not uncommon for such protocols to distribute slashed funds to honest operators. So, considering this could be the case, that logic would be implemented as stated by the comments:

https://github.com/code-423n4/2024-07-karak/blob/main/src/SlashingHandler.sol#L57

    function handleSlashing(IERC20 token, uint256 amount) external {
        if (amount == 0) revert ZeroAmount();
        if (!_config().supportedAssets[token]) revert UnsupportedAsset();

        SafeTransferLib.safeTransferFrom(address(token), msg.sender, address(this), amount);
        // Below is where custom logic for each asset lives
        SafeTransferLib.safeTransfer(address(token), address(0), amount);
    }

So, considering a DSS that allows an operator to be overleveraged, it will be susceptible to this attack.

The attacker would follow these steps:

  1. Deploy a malicios DSS and stake own vault to it.
  2. Stake vault to the target honest DSS.
  3. Perform malicios operation on the target DSS. This would trigger a slashing request on the honest DSS side.
  4. Frontrun the honest DSS slashing request, by submitting a slashing request from the malicios DSS for 100%. This doesn't need to be in the same block, so it would work on non-mempool networks as well.
  5. Finalize the slashing request from the malicios DSS, retrieving the staked funds.

As a result the honest DSS would slash 0 funds, since the operator would have remove his own funds via the malicios DSS slashing request.

Tools Used

Manual Review

Recommended Mitigation Steps

This would be complicated to solve, however options are requiring an operator to always have a non-leveraged stake in a DSS. This way they will always lose funds should they attempt this attack vector. Or instead limit slashing to less than 100% of vault stake, for example 80% max slashing for a DSS.

Assessed type

Other

@c4-bot-10 c4-bot-10 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Jul 30, 2024
c4-bot-10 added a commit that referenced this issue Jul 30, 2024
@c4-bot-7 c4-bot-7 changed the title Malicios Operator with can utilize a malicios DSS and an overleveraged vault to frontrun an honest DSS's slashing request to protect its funds Malicios Operator can utilize a malicios DSS and an overleveraged vault to frontrun an honest DSS's slashing request to protect its funds Jul 30, 2024
@c4-bot-12 c4-bot-12 added the 🤖_06_group AI based duplicate group recommendation label Jul 30, 2024
@howlbot-integration howlbot-integration bot added the insufficient quality report This report is not of sufficient quality label Aug 1, 2024
@c4-judge
Copy link
Contributor

MiloTruck changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Aug 11, 2024
@MiloTruck
Copy link

MiloTruck commented Aug 11, 2024

This is a good attack scenario, but unfortunately it relies on speculation of future code:

The default implementation of the slashing handler in the protocol burns the slashed funds, however, it is common practice in these types of systems, to distribute the slashed funds to honest operators, by for example sending them first to a ledger on the DSS side. Other slashing handlers are expected to be made.

The current SlashingHandler in-scope sends the funds to the zero address. This scenario relies on slashing handler behavior not implemented in the current code, and as such, is hypothetical and QA at best.

Note that slashing handlers are whitelisted by the protocol's MANAGER_ROLE.

@c4-judge c4-judge reopened this Aug 12, 2024
@c4-judge
Copy link
Contributor

MiloTruck marked the issue as grade-b

@gesha17
Copy link

gesha17 commented Aug 14, 2024

Hello @MiloTruck,

Thank you for judging ser.

I believe this submission still correctly indentifies that it is not possible to deploy a slashing handler that does anything else than just burn the slashed tokens. So at the very least this is a limitation of the current code and potential high severity vulnerability if the limitation is ignored or not noticed.

In other words, the currently deployed slashing handler does not make the protocol vulnerable to this attack, but the rest of the system is limited to only using this slashing handler because of the attack vector. Because of this I still believe this should be at least medium severity.

Also, I would like to point out the root cause being the ability of any DSS to specify the slashing of 100% of funds and vaults being overleveraged, these two would make the deployment of slashing handlers with custom logic not possible, and impair future development of the protocol. According to C4 docs that should make this eligible to be in scope.

Also, the Karak Docs say that slashing handlers that do not burn the tokens, but distribute the funds to the affected parties can be deployed in the future. If this attack is used the funds also won't reach the affected parties. So, considering it likely that a slashing handler that is different to the current one and that does not burn the tokens is deployed, it makes this attack vector possible and this should be high severity...

@MiloTruck
Copy link

MiloTruck commented Aug 20, 2024

I would like to refer you to the following C4 ruling: code-423n4/org#72

Additionally, if this were to happen, it is reasonable to assume the slashing veto committee would simply veto the slashing request from the operator as it is obviously malicious.

This remains as QA.

@gesha17
Copy link

gesha17 commented Aug 20, 2024

@MiloTruck

Yes, I read the verdict I believe it is still medium or high severity. It is the same text as the link to the C4 docs that I sent in my escalation comment... It doesn't exclude the finding from being in scope or medium/high severity

Warden may make an argument on why a future code change that would make the bug manifest is reasonably likely

Both Karak docs and the code comments suggest that such custom logic slashing handlers will be deployed eventually. The dev team will have to either not deploy those slashing handlers or make changes to the codebase to address this vulnerability before deploying them.

To your comment:

Additionally, if this were to happen, it is reasonable to assume the slashing veto committee would simply veto the slashing request from the operator as it is obviously malicious.

An attacker that attempts this will have knowledge of how to deploy and run an Operator and a DSS and attempt a malicios action(exploit) against an honest DSS with potential loss of funds for it's users. So, I think it is fair to assume that the attacker will be well funded and able to create a decoy "honest" DSS to bypass the veto committee with a non-negligible probability. That should be of concern to the protocol team. If the veto committee does not veto this, then it will certainly lead to loss of funds and have 0 impact on the attacker.

The mitgation steps that I recommend, at least guarantee loss of funds for the attacker no matter if the veto committee notices the malicios slashing event.

@C4-Staff C4-Staff added the Q-11 label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden grade-b insufficient quality report This report is not of sufficient quality Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_06_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

7 participants