Dopex - lsaudit's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L27 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L104 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L270 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L572-L583

Vulnerability details

Impact

According to documentation, strikes are represented in 1e8 precision:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

/// @dev Option tokens are in erc20 18 decimals & Strikes are in 1e8 precision

However, strikes are calculated with a help of roundUp function whose precision is set as: 1e6.

File: contracts/perp-vault/PerpetualAtlanticVault.sol

uint256 public roundingPrecision = 1e6;

Calculations in improper precision may lead to financial loss - since improper rounding will be used.

Proof of Concept

While strikes precision should be 1e8, it's actually 1e6:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

 uint256 strike = roundUp(currentPrice - (currentPrice / 4)); // 25% below the current price

The implementation of roundUp is as follows:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

   * @dev Function to round up a value to the roundingPrecision.
   * @param _strike the strike
   * @return strike rounded up to the nearest roundingPrecision
   **/
  function roundUp(uint256 _strike) public view returns (uint256 strike) {
    uint256 remainder = _strike % roundingPrecision;
    if (remainder == 0) {
      return _strike;
    } else {
      return _strike - remainder + roundingPrecision;
    }
  }

As described in the comment: @return strike rounded up to the nearest roundingPrecision, which is confirmed in this line:

uint256 remainder = _strike % roundingPrecision;

roundingPrecision is set to 1e6:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

uint256 public roundingPrecision = 1e6;

As it was mentioned above, roundingPrecision is set to 1e6, instead of 1e8, thus strikes are represented in 1e6 precision, instead of 1e8.

Tools Used

Manual code review, Remix IDE

Recommended Mitigation Steps

Change roundingPrecision to 1e8

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L572-L583 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1189-L1190 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L270

Vulnerability details

Impact

According to documentation and comments inside the code-base - strike should be 25% below current price. This condition, however, does not occur when current price is very small.

Strikes calculations are based on roundUp, which rounds up even small values to a very big number. Whenever current price is very small, the strikes' price value will be overinflated. Since strikes are calculated based on the current price (and they should be 25 % below current price) - due to roundUp, its value will be overinflated. Because of the roundUp implementation, the discrepancy between calculated strike and the price will be too big.

Proof of Concept

roundUp is implemented as below:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

   * @dev Function to round up a value to the roundingPrecision.
   * @param _strike the strike
   * @return strike rounded up to the nearest roundingPrecision
   **/
  function roundUp(uint256 _strike) public view returns (uint256 strike) {
    uint256 remainder = _strike % roundingPrecision;
    if (remainder == 0) {
      return _strike;
    } else {
      return _strike - remainder + roundingPrecision;
    }
  }

and roundingPrecision is defined as uint256 public roundingPrecision = 1e6;. This implies, that any strike less than 1e6, will be rounded to 1e6 (and above).

According to comment section, strike should be 25% below the current price:

File: contracts/core/RdpxV2Core.sol

    uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price

File: contracts/perp-vault/PerpetualAtlanticVault.sol

uint256 strike = roundUp(currentPrice - (currentPrice / 4)); // 25% below the current price

However, based on the roundUp implementation, it's more than clear, that for some rdpxPrice and currentPrice, the result will be nowhere near being 25% below. Some example values which demonstrates that the discrepancy between strike's price and current price is enormous:

roundUp(1) = 1000000 
roundUp(100) = 1000000 
roundUp(10000001) = 11000000 
roundUp(20000123) = 21000000

Tools Used

Manual code review

Recommended Mitigation Steps

Reconsider the implementation of roundUp, so that for values of strike won't be extensively larger than they should be.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

According to documentation, PerpetualAtlanticVaultLP.sol is:

Contract for the Perpetual Atlantic Vault LP (ERC4626)

Almost every ERC4626 and ERC4626-like contracts are vulnerable to share inflation attack. In the current implementation, there is no protection from this type of attack, thus contract is considered as vulnerable.

Proof of Concept

Functions redeem allows to redeem ERC4626 token, while function deposit allows to deposit into ERC4626. These two functions will be used to perform the attack.

1. The new Perpetual Atlantic Vault LP is being deployed, the attacker notices that and is the first person who deposits 1 asset into the contract.
2. As 1 asset is being deployed, he receives 1 share - function convertToShares states how many shares will be received:

283: supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);

since attacker is the first depositor, supply is 0, thus number of shares is equal to number of assets.

3. Now, the victim sees the vault and decides to deposit 1000 assets.

4. Now, the attacker fronts-runs the victim and deposits another 1000 assets, now, in the vault there are 1001 assets.

5. When victim's deposit enters the vault, the share calculation is calculated as: assets.mulDivDown(supply, totalVaultCollateral) However, because of the front-run, totalVaultCollateral > supply. And as we have denominator larger than the numerator, mulDivDown rounds the result to 0. Victim deposited 1000 assets, but he will get 0 shares.

Tools Used

Manual code review

Recommended Mitigation Steps

The initial deposit should burn the shares instead of allowing the first depositor to redeem them. Burning the first share will stop the first depositor from setting up the contract in a state with ratio vulnerable to share inflation attack.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L61 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L73 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L240-L290

Vulnerability details

Impact

While it's impossible to add two assets with the same addresses - it's possible to add two assets with the same symbol. Adding assets to the reserves does not check if their symbol is not already there. Having two assets with the same symbols breaks down adding/removing functionality. When asset with the same symbol is added - it won't be possible to remove it later.

Proof of Concept

While addAssetTotokenReserves does check if asset with the same address is not being added:

246:    for (uint256 i = 1; i < reserveAsset.length; i++) {
247:      require(
248:        reserveAsset[i].tokenAddress != _asset,
249:        "RdpxV2Core: asset already exists"
250:      );

there is no additional check, to verify if asset with the same symbol is being added.

This behavior allows to add asset with different address, but with the same symbol.

As PoC, we will be using Remix IDE. Clone the whole repo into the Remix IDE, then compile and deploy RdpxV2Core.sol. Now, inside Remix IDE, call functions as follows:

addAssetTotokenReserves(0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, "AAA")
addAssetTotokenReserves(0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2, "BBB")
addAssetTotokenReserves(0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db, "CCC")
addAssetTotokenReserves(0x78731D3Ca6b7E34aC0F824c42a7cC18A495cabaB, "DDD")
addAssetTotokenReserves(0xdD870fA1b7C4700F2BD7f44238821C26f7392148, "BBB")

We have created two assets with the same symbol: BBB. This can be confirmed by calling reserveAsset:

There are two BBB assets:

reserveAsset(2):
    0:
    address: tokenAddress 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol BBB

reserveAsset(5):
    0:
    address: tokenAddress 0xdD870fA1b7C4700F2BD7f44238821C26f7392148
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol BBB

However, while calling the reservesIndex, it will return index of the last one:

reservesIndex("BBB"):
    0:
    uint256: 5

Now, as we have two assets with the same symbol, we will try to remove asset with BBB symbol by calling removeAssetFromtokenReserves.

removeAssetFromtokenReservers("BBB")

Let's see what's happening after calling this function.

1. reservesIndex still returns 5 (even though asset had been removed):

reservesIndex("BBB"):
    0:
    uint256: 5

2. Previously created BBB asset still exists:

reserveAsset(2):
    0:
    address: tokenAddress 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol BBB

This incoherence (reservesIndex returns different values than reserveAsset) breaks the functionality of removeAssetFromtokenReservers.

Let's try to call removeAssetFromtokenReservers("BBB") once again, to remove the previously added BBB. Unfortunately, calling this function again leads to revert:

removeAssetFromtokenReservers("BBB")

transact to RdpxV2Core.removeAssetFromtokenReserves errored: VM error: revert.

This implies, that we cannot remove previously added BBB asset. But let's investigate the behavior of protocol even more.

Now, we will try to add another asset:

addAssetTotokenReserves(0x1aE0EA34a72D944a8C7603FfB3eC30a6669E454C, "EEE")

Let's confirm that it has been added correctly:

reservesIndex("EEE"):
    0:
    uint256: 5

reserveAsset(5):
    0:
    address: tokenAddress 0x1aE0EA34a72D944a8C7603FfB3eC30a6669E454C
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol EEE

Now, let's try to remove BBB again

removeAssetFromtokenReservers("BBB")

This time, transaction succeeded, however, something strange happened. Let's examine the whole reserveAsset :

reserveAsset(1):
    0:
    address: tokenAddress 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol AAA

reserveAsset(2):
    0:
    address: tokenAddress 0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol BBB   

reserveAsset(3): 
    0:
    address: tokenAddress 0x4B20993Bc481177ec7E8f571ceCaE8A9e22C02db
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol CCC

reserveAsset(4):
    0:
    address: tokenAddress 0x78731D3Ca6b7E34aC0F824c42a7cC18A495cabaB
    1:
    uint256: tokenBalance 0
    2:
    string: tokenSymbol DDD

reserveAsset(5):
call to RdpxV2Core.reserveAsset errored: VM error: revert.

As it's demonstrated above, BBB still exists - calling removeAssetFromtokenReservers("BBB") removed asset EEE instead!

This behavior proves, that when we will insert two assets with the same symbol, it won't be possible to remove one of them.

Tools Used

Manual code review, Remix IDE

Recommended Mitigation Steps

Do not allow to insert assets with the same symbol. Change require condition from:

246:    for (uint256 i = 1; i < reserveAsset.length; i++) {
247:      require(
248:        reserveAsset[i].tokenAddress != _asset,
249:        "RdpxV2Core: asset already exists"
250:      );

to

246:    for (uint256 i = 1; i < reserveAsset.length; i++) {
247:      require(
248:        reserveAsset[i].tokenAddress != _asset &&  reserveAsset[i].tokenSymbol != _assetSymbol,
249:        "RdpxV2Core: asset already exists"
250:      );

Severity Evaluation

Adding new assets to token reserves is possible only by user with administrative rights. Role DEFAULT_ADMIN_ROLE is needed to call addAssetTotokenReserves. However, since there are a lot of tokens with very similar symbols - or even tokens with different addresses but with the same symbols (in most cases, asset symbol consist of only 3 letters, so it's obvious that some symbols will overlap) - it's very easily to either miss-type the token symbol, or accidently provide the token with the same symbol which had already been added. This - which seems to be harmless mistake - may lead to enormous consequences. The full functionality of adding and removing new assets will be broken. Since the main functionality of the protocol will be broken - the severity of this issue had been estimated as High.

Assessed type

Invalid Validation