Dopex - Mike_Bello90's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315-L369 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L372-L396

Vulnerability details

Impact

The functions settle & payFunding are using the onlyRole(RDPXV2CORE_ROLE) modifier so that only the RdpxV2Core contract can use these functions but additionally they are also using the _isEligibleSender function that checks if a contract is whitelisted this is like having 2 sensitive roles for a contract which could lead to access control or Dos vulnerabilities in the contract.

Proof of Concept

if these functions are meant to be used only by the RdpxV2Core contract, then it's not necessary to add the _isEligibleSender function because this requires adding the RdpxV2Core to the whitelisted contracts and if any other whitelisted contract (not the RdpxV2Core) is going to be used to call this functions then you should give this or these other contracts the RDPXV2CORE_ROLE and this could lead to vulnerabilities in the contract.

there's no problem adding the RdpxV2Core contract to the whitelisted contracts, but if you have a whitelisted contract that you want it to be able to use only these 2 functions(settle & payFunding) you will have to give the whitelisted contracts the RDPXV2CORE_ROLE and this will also give access to this contract to the whole RDPXV2CORE_ROLE functionality of the protocol, so if this contract is malicious or get compromised, the whole RDPXV2CORE_ROLE will get compromised too.

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315-L369

function settle(uint256[] memory optionIds)
        external
        nonReentrant
        onlyRole(RDPXV2CORE_ROLE)
        returns (uint256 ethAmount, uint256 rdpxAmount)
    {
        _whenNotPaused();
        _isEligibleSender();

        updateFunding();

        for (uint256 i = 0; i < optionIds.length; i++) {
            uint256 strike = optionPositions[optionIds[i]].strike;
            uint256 amount = optionPositions[optionIds[i]].amount;

            // check if strike is ITM
            _validate(strike >= getUnderlyingPrice(), 7);

            ethAmount += (amount * strike) / 1e8;
            rdpxAmount += amount;
            optionsPerStrike[strike] -= amount;
            totalActiveOptions -= amount;

            // Burn option tokens from user
            _burn(optionIds[i]);

            optionPositions[optionIds[i]].strike = 0;
        }

        // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
        collateralToken.safeTransferFrom(addresses.perpetualAtlanticVaultLP, addresses.rdpxV2Core, ethAmount);
        // Transfer rdpx from rdpx rdpxV2Core to perpetual vault

        IERC20WithBurn(addresses.rdpx).safeTransferFrom(
            addresses.rdpxV2Core, addresses.perpetualAtlanticVaultLP, rdpxAmount
        );

        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(ethAmount);
        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).unlockLiquidity(ethAmount);
        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(rdpxAmount);

        emit Settle(ethAmount, rdpxAmount, optionIds);
    }

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L372-L396

function payFunding() external onlyRole(RDPXV2CORE_ROLE) returns (uint256) {
        _whenNotPaused();
        //@audit porque usan este modifier si solo lo puede llamar el RDPXV2CORE_ROLE??
        _isEligibleSender();
        //@audit posible DOS? enviando minimo amout of eth o rdpx?
        _validate(totalActiveOptions == fundingPaymentsAccountedFor[latestFundingPaymentPointer], 6);

        collateralToken.safeTransferFrom(
            addresses.rdpxV2Core, address(this), totalFundingForEpoch[latestFundingPaymentPointer]
        );
        _updateFundingRate(totalFundingForEpoch[latestFundingPaymentPointer]);

        emit PayFunding(msg.sender, totalFundingForEpoch[latestFundingPaymentPointer], latestFundingPaymentPointer);

        return (totalFundingForEpoch[latestFundingPaymentPointer]);
    }

Tools Used

Manual Code Review

Recommended Mitigation Steps

It's recommended to always use more granular access control rules, guide you for the Principle of Least Privilege (POLP), and try to avoid these redundant privileges.

If only the RdpxV2Core contract can call these functions, then delete the _isEligibleSender(); lines in these functions or if you need that these functions can be called only by a whitelisted contract that is not necessary to have the RDPXV2CORE_ROLE, then delete the onlyRole modifier.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L764-L783 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L315-L369

Vulnerability details

Impact

An attacker can break the settlement mechanism of the protocol preventing being liquidated and preventing the settlement of all the options of an epoch for very low cost.

Proof of Concept

When the protocol has to settle the options the Admin calls the Settle function in the RdpxV2Core contract, which then calls the Settle function in the PerpetualAtlanticVault contract and this calls the subtractLoss function in the PerpetualAtlanticVaultLP contract, the subtractLoss function has to subtract the loss from the collateral (WETH) but first require the balance of the PerpetualAtlanticVaultLP contract has to be equal to totalCollateral - loss, this open the door to an attacker to brick the settlement mechanism.

let's imagine that an epoch is about to finish and an attacker is about to be liquidated or he just wants to harm the protocol.

The attacker can transfer 1 wei of WETH to the PerpetualAtlanticVaultLP contract this will cause the require require(collateral.balanceOf(address(this)) == _totalCollateral - loss, "Not enough collateral was sent out"); in the subtractLoss function to revert the Tx for all the options the protocol is trying to settle.

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

To show bug I just added the following lines to the testSettle() function in the unit.t.sol contracts of perp-vault and rdpxV2Core tests.

console.log("sending 1 wei to vaultLp...");
weth.transfer(address(vaultLp), 1);

and the result show the settle function reverted by the require of the subtractLoss function:

 % forge test --match-test testSettle -vv
[⠆] Compiling...
[⠒] Compiling 1 files with 0.8.19
[⠑] Solc 0.8.19 finished in 2.37s
Compiler run successful!

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 890276)
Logs:
  sending 1 wei to vaultLp...

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 956.60ms

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 2946563)
Logs:
  sending 1 wei to vaultLp...

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 956.59ms
Ran 2 test suites: 0 tests passed, 2 failed, 0 skipped (2 total tests)

Failing tests:
Encountered 1 failing test in tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 890276)

Encountered 1 failing test in tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 2946563)

Encountered a total of 2 failing tests, 0 tests succeeded

Tools Used

Manual Code Review

Recommended Mitigation Steps

One way to avoid this bug is simple change the == operator to >= in the require subtractLoss function, like this:

require(collateral.balanceOf(address(this)) >= _totalCollateral - loss, "Not enough collateral was sent out");

Assessed type

DoS