Dopex - GangsOfBrahmin's results

Lines of code

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

Vulnerability details

Impact

ethAmount subtracted can be greater than the totalAvailableCollateral.

Proof of Concept

In PerpetualAtlanticVault.settle function transfer of collateral token occur from perpetual vault to rdpxV2Core further it calling PerpetualAtlanticVaultLP.subtractLoss to subtract ethAmount transfer from vault to rdpxV2Core

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

which is subtracting ethAmount from _totalCollateral in PerpetualAtlanticVaultLP

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

the problem here is that before subtracting it donot check the ethAmount to subtract is greater then the totalCollateral or not like in purchase function it validate the ethAmount should be less and equal to totalAvailableCollateral()

    uint256 requiredCollateral = (amount * strike) / 1e8;

    _validate(
      requiredCollateral <= perpetualAtlanticVaultLp.totalAvailableCollateral(),
      3
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L275C4-L281C7

not checking ethAmount before subtract might revert whole settle function to its initial state.

Tools Used

Manual

Recommended Mitigation Steps

Add a validate check before subtracting ethAmount , _validate( requiredCollateral <= perpetualAtlanticVaultLp.totalAvailableCollateral(), 3 );

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

ERC4626 vault share price can be maliciously inflated on the initial deposit, leading to the next depositor losing assets due to precision issues.

The first depositor of an ERC4626 vault can maliciously manipulate the share price by depositing the lowest possible amount (1 wei) of liquidity and then artificially inflating ERC4626.totalAssets.

This can inflate the base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as a base and worst case, due to rounding down, if this malicious initial deposit front-run someone else depositing, this depositor will receive 0 shares and lost his deposited assets.

Proof of Concept

Given a vault with DAI as the underlying asset:

Alice (attacker) deposits initial liquidity of 1 wei DAI via deposit() Alice receives 1e18 (1 wei) vault shares Alice transfers 1 ether of DAI via transfer() to the vault to artificially inflate the asset balance without minting new shares. The asset balance is now 1 ether + 1 wei DAI -> vault share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18) Bob (victim) deposits 100 ether DAI Bob receives 0 shares Bob receives 0 shares due to a precision issue. His deposited funds are lost.

The shares are calculated as following return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets()); In case of a very high share price, due to totalAssets() > assets * supply, shares will be 0.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a well-known issue, Uniswap and other protocols had similar issues when supply == 0.

For the first deposit, mint a fixed amount of shares, e.g. 10**decimals()

    return 10**decimals; 
} else {
    return assets.mulDivDown(supply, totalAssets());
}

Assessed type

ERC4626