Dopex - zzebra83's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/amo/UniV3LiquidityAmo.sol#L361 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1185 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1110 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L1106 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/reLP/ReLPContract.sol#L306 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L780 https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/core/RdpxV2Core.sol#L803

Vulnerability details

Impact

The Sync function plays a critical role in overall wellbeing of the protocol by syncing the backing reserves within the Core contract. It is called by the Uniswap AMOS for instance whenever they want to add liquidity into the backing reserve, or by the ReLP contract whenever it wants to send rdpx to the backing reserve.

As mentioned previously, the sync function is extremely important, as it is responsible for updating the reserve asset mapping to reflect the liquidity of the backing reserve.

However a user who delegates eth can either unintentionally or maliciously permanently DOS the ability to call this function. This can be done by manipulating the value of the totalWethDelegated variable.

  function sync() external {
// @audit syncs Backing reserves with contract balances
for (uint256 i = 1; i < reserveAsset.length; i++) {
  uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
    address(this)
  );

  if (weth == reserveAsset[i].tokenAddress) {
    // @audit if totalWethDelegated is inflated, this function will never
    // work, complete DOS because line below will cause a revert with underflow
    balance = balance - totalWethDelegated;
  }
  reserveAsset[i].tokenBalance = balance;
}

emit LogSync();

}

The user can increase the value of totalWethDelegated by calling the addToDelegate. The user can then call withdraw which allows him to withdraw unused WETH that he delegated. the oversight is that the totalWethDelegated variable is not updated to handle this withdrawal, ergo the balance of WETH in contract will be reduced by the amount of withdrawn while totalWethDelegated will still be inflated causing the calculation to update balance of weth in sync function to revert due to underflow.

The consequence of this is obvious, the sync function will be DOS'ed. The impact is quite severe since AMOS will unable to provide liquidity to the core contract. RELP functionality will also be bricked. in addition, if the sync function wont work, then internal operations within the core contract will be affected. for instance the lowerDepeg function responsible for ensuring the peg dpxETH to ETH on the curve pool is maintained could also be compromised if the rdpx tokenbalance in the reserverAsset mapping can not be properly updated via a sync operation, leading to an underflow if _rdpxAmount amount to deduct is larger than the RDPX token balance.

  // @audit this line in the lowerDepeg might fail if the tokenBalance was not updated via the syn function
  reserveAsset[reservesIndex["RDPX"]].tokenBalance -= _rdpxAmount; 

Proof of Concept

    function testManipulateWethBalance() public {
/// add to delegate with different fees
uint256 userBalance = weth.balanceOf(address(this));
uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);
uint256 delgateId2 = rdpxV2Core.addToDelegate(10 * 1e18, 20 * 1e8);
uint256 delgateId3 = rdpxV2Core.addToDelegate(10 * 1e18, 30 * 1e8);

// assert that the user balance is reduced by the amount
assertEq(weth.balanceOf(address(this)), userBalance - 70 * 1e18);
assertEq(rdpxV2Core.totalWethDelegated(), 70 * 1e18);

rdpxV2Core.withdraw(delegateId);
rdpxV2Core.withdraw(delgateId2);
rdpxV2Core.withdraw(delgateId3);

// assert that the user balance is back to origin
assertEq(weth.balanceOf(address(this)), userBalance);
// assert total weth delegated variable is still the same..
assertEq(rdpxV2Core.totalWethDelegated(), 70 * 1e18);

vm.expectRevert(stdError.arithmeticError);

rdpxV2Core.sync();

}

The POC above clearly highlights this attack. in a single transaction, a malicious user for instance can delegate and then undelegate leaving the totalWethDelegated variabled inflated. the call to sync will fail with arithmetic underflow error. The malicious user can for instance take out a flash loan for a large amount of ETH, and use that to severely inflate the totalWethDelegated. they can within the same transaction immediately withdraw the delegated ETH.

Tools Used

Manual Review

Recommended Mitigation Steps

The Fix is obvious, make sure the totalWethDelegated is updated properly to reflect the current balance after a user withdraws any delegated ETH.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/perp-vault/PerpetualAtlanticVault.sol#L362-L363

Vulnerability details

Impact

Any time an option is purchased, the collateral requirement to pay for any losses is effectively locked in the perpetualAtlanticVaultLp.

  perpetualAtlanticVaultLp.lockCollateral(requiredCollateral);

If option writers attempt to redeem their assets that they deposited in the Perpetual Atlantic Vault LP, the transaction will fail if the vault has too much collateral locked up.

// @audit if locked up collateral is too high, they cant get their full assets
beforeWithdraw(assets, shares);

function above triggers the check below:

  function beforeWithdraw(uint256 assets, uint256 /*shares*/) internal {
require(
  assets <= totalAvailableCollateral(),
  "Not enough available assets to satisfy withdrawal"
);
_totalCollateral -= assets;

}

totalAvailableCollateral is equal to totalCollateral minus the locked up collateral. Essentially what this means is that if the locked up collateral is sufficiently large, then user potentially can not redeem their assets from the LP.

the crux of problem here is that if the perpetual option can never be settled because the price keeps moving in a direction where option is out of the money, the collateral allocated for it from the LP is never unlocked.

Just to be clear, an OTM put can not be settled obviously, the settlement of the option via the settle function in the PerpetualAtlanticVault contract will immediately revert when the validation line is hit:

_validate(strike >= getUnderlyingPrice(), 7);

Upon review of the code base there is no mechanism within the Core Contract and the PerpetualAtlanticVault contract to 'forfeit' options ie to unlock collateral linked to options that can not be settled. this is in clear delineation of the docs which clearly state the following:

Users depositing into this APP contract can redeem their deposit when collateral is available (when the core contract buys options collateral is locked until the Core Contract settles or forfeits options).

The impact of this oversight can be demonstrated in a simple example. assume the price of rdpx is 0.1 ETH. any options written for a strike price of 0.05 will never be able to settled if the price of RDPX stays above 0.05. This essentially means that any collateral locked up for options written with a strike price of 0.05 will continue to be locked up. Overtime it is reasonable to expect that if the price of the RDPX continues to grow, then the amount of locked up collateral will also continue to grow as well, with a very small possibility to unlock due to the relatively very low strike price(deep out of the money options). It is reasonable to expect this can cause situations where option writers will not be able to withdraw their funds.

Proof of Concept

Issue is self explanatory.

Tools Used

Manual Review

Recommended Mitigation Steps

As the docs rightly suggest, need to implement a mechanism within the core contract whereby options can be forfeited via authorized users, and upon forfeit, the locked up collateral in LP associated with these forfeit options is freed.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/amo/UniV2LiquidityAmo.sol#L160-L178

Vulnerability details

Impact

The UniV2LiquidityAmo and UniV3LiquidityAmo play critical role in controlling how much assets are in the backing reserve.

The admin of the AMOS can perform add/remove liquidity or swap operations that all make a call to the _sendTokensToRdpxV2Core function which then proceeds to transfer tokens to the core contract to update the backing reserve, which is represented in the contract as a map called reserveAsset. the _sendTokensToRdpxV2Core in UniV3LiquidityAmo as an example does this functionality as seen below:

  function _sendTokensToRdpxV2Core(address tokenA, address tokenB) internal {
uint256 tokenABalance = IERC20WithBurn(tokenA).balanceOf(address(this));
uint256 tokenBBalance = IERC20WithBurn(tokenB).balanceOf(address(this));
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(tokenA).safeTransfer(rdpxV2Core, tokenABalance);
IERC20WithBurn(tokenB).safeTransfer(rdpxV2Core, tokenBBalance);

// sync token balances
IRdpxV2Core(rdpxV2Core).sync();

emit LogAssetsTransfered(tokenABalance, tokenBBalance, tokenA, tokenB);

}

notice that after the tokens are transferred, a call is made to the sync function which is reposnsible for syncing the actual balance of a token with the amont stored in the backing reserve as seen below:

  function sync() external {
// @audit syncs Backing reserves with contract balances
for (uint256 i = 1; i < reserveAsset.length; i++) {
  uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
    address(this)
  );

  if (weth == reserveAsset[i].tokenAddress) {
    balance = balance - totalWethDelegated;
  }
  reserveAsset[i].tokenBalance = balance;  @audit update backing reserve
}

emit LogSync();

}

Accordingly, the bug is evident in the _sendTokensToRdpxV2Core implemented in the UniV2LiquidityAmo version as seen below:

  function _sendTokensToRdpxV2Core() internal {
uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
  address(this)
);
uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
  address(this)
);
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(addresses.tokenA).safeTransfer(
  addresses.rdpxV2Core,
  tokenABalance
);
IERC20WithBurn(addresses.tokenB).safeTransfer(
  addresses.rdpxV2Core,
  tokenBBalance
);

// @audit missing sync function call here. 

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

}

Notice the call to sync us missing, which means the balance of tokens will be increased, but will not be reflected in the backing reserve.

The impact of this oversight is that while the add/remove liquidity or swap operations might succeed, they will fail to actually update the backing reserve. this might influence the proper functioning of the protocol. the impact however is mitigated by the fact, that even though the sync call is missing, upon realisation that the backing reserves are out of sync, anyone can then call the sync function to update the backing reserve.

Proof of Concept

Issue is self explanatory.

Tools Used

Manual Review

Recommended Mitigation Steps

fix is simple, just need to include the call the sync function as seen below:

      function _sendTokensToRdpxV2Core() internal {
uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
  address(this)
);
uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
  address(this)
);
// transfer token A and B from this contract to the rdpxV2Core
IERC20WithBurn(addresses.tokenA).safeTransfer(
  addresses.rdpxV2Core,
  tokenABalance
);
IERC20WithBurn(addresses.tokenB).safeTransfer(
  addresses.rdpxV2Core,
  tokenBBalance
);

// @audit add missing sync function call here. 
// sync token balances
IRdpxV2Core(rdpxV2Core).sync();

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

}

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/perp-vault/PerpetualAtlanticVault.sol#L333

Vulnerability details

Impact

According to the Docs, when an option position expires ITM(in the money), option writers pay option buyers the difference between strike and settlement prices thus incurring a loss. an example is if a put option is written with a strike price 0f 0.05. if the price at settlement is 0.045, then it is considered in the money(ITM). if the strike and settlement price are equal, it is at the money. there is no incentive to exercise a ATM option because option writers only pay option buyers the difference between strike and settlement prices as mentioned before.

There are two significant problems with how this is implemented in the Perpetual Atlantic Vault Settlement Logic. first, the validation of whether an option is in the money is invalid.

 _validate(strike >= getUnderlyingPrice(), 7);

the validation above will pass for ITM and ATM money options.

Second and more importantly, the actual loss amount calculated on settlement is also incorrect. but before we discuss that lets first go back to the purchase functionality which enables core to buy options to help us understand the flaw better. that purchase functionality correctly ensures the vault has enough collateral to cover the option purchase. this is done via the following formula:

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

this collateral is then locked up in the vault, to specify its being designated as collateral used to cover any losses in the option position which the writers will pay up on any settlement. now lets go back to the settlement process.

on settlement the loss amount deducted is based on the ethAmount which is calculated as follows:

ethAmount += (amount * strike) / 1e8;

this formula is incorrect and actually represents the entirety of locked up collateral, rather it should be as follows:

ethAmount += (amount * (strike - getUnderlyingPrice())) / 1e8;

the loss is not equal to the total amount multiplied by the strike(which is actually equal to the total required collateral that is needed to create the option), but rather the amount multiplied by the difference between the strike and the price at settlement( e.g. 0.05-0.045=0.005).

this loss amount will then be transferred out from the LP vault and then deducted from the total collateral via the function below:

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

The impact of this discrepancy is that a significant portion of option settlement process is flawed. not only do at the money options cause option writers to suffer losses they do not deserve, but also it seems at settlement whether its for ITM or ATM options, they get to lose the entirety of the locked up collateral since the amount of collateral deducted from the LP vault is not equal to the difference between the strike and settlement price, but to the full strike price(ie the full required collateral locked up to create the option position).

It is very reasonable to expect that this high inflation in collateral losses will accumulate and drain the vault of necessary collateral and will negatively impact option writers. The funding requirements of the Vault will also be significantly higher.

finally there is a third problem as well that is more subtle. consider the line below which unlocks the collateral:

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

Given that the correct loss amount is recorded, which effectively reducded the totalCollateral in the LP by the loss amount, the amount to unlock should be equal to ethAmount-lossAmount.

Proof of Concept

  function testRedeem() external {
rdpx.mint(address(1), 10 ether);
weth.mint(address(1), 1 ether);

deposit(1 ether, address(1));

(, uint256 id) = vault.purchase(1 ether, address(this)); // purchases for epoch 1; send 0.05 weth premium
uint256[] memory optionIds = new uint256[](1);
optionIds[0] = id;

skip(86500); // expire
priceOracle.updateRdpxPrice(0.010 gwei);
vault.updateFundingPaymentPointer();

uint256[] memory strikes = new uint256[](1);
strikes[0] = 0.015 gwei;
uint256 fundingAccrued = vault.calculateFunding(strikes);
vault.payFunding();

assertEq(fundingAccrued, 0.05 ether);
assertLe(
  1 ether + 0.05 ether - vaultLp.totalCollateral(),
  1000 gwei // block.timestamp ensued dust
); // initial deposit + epoch 1 funding

priceOracle.updateRdpxPrice(0.0 gwei); // @audit price set to zero, but all assertions below still pass. passing test highlights severe underlying logic problems with test and the implementation code 
(uint256 ethAmount, uint256 rdpxAmount) = vault.settle(optionIds);

assertEq(ethAmount, 0.15 ether);
assertEq(rdpxAmount, 1 ether);

vm.startPrank(address(1), address(1));
vaultLp.approve(address(vault), 0.1 ether); // for coverage
vaultLp.approve(address(vault), type(uint256).max);
uint256 balance = vaultLp.balanceOf(address(1)); // shares received from vault. originall 1 eth
vm.expectRevert("ZERO_ASSETS");
(ethAmount, rdpxAmount) = vaultLp.redeem(0, address(1), address(1));

vm.expectRevert();
vaultLp.redeem(1, address(1), address(this));
skip(86400);
vault.updateFunding(); 
skip(400);
(ethAmount, rdpxAmount) = vaultLp.redeem(balance, address(1), address(1)); // @audit an losse
assertGt(ethAmount, 0.9499999999 ether); // asset settled, lose 0.05 ether (0.15 strike - 0.1 current price); receive 1 rdpx
assertLt(ethAmount, 0.95 ether); // 1 to 3 gwei lost in precision
assertEq(rdpxAmount, 1 ether);
vm.stopPrank();

}

The test above was pulled from the repo(perp-vault/Unit.t.sol), the only thing modified was line 194:

priceOracle.updateRdpxPrice(0.0 gwei);

I changed the rdpx price from 0.01 to 0. it makes no sense for the test to pass if the price of rdpx is set to zero, one would expect a different result. a put option that was exercised with a settlement price of 0 would signal a higher loss than one exercised with a price closer to the exercise price. however the amount of eth redeemed is the same:

 assertGt(ethAmount, 0.9499999999 ether); // asset settled, lose 0.05 ether (0.15 strike - 0.1 current price), @audit, this assertions succeeds at any price below exercise, makes no sense.

the line above from the test seems spurious and is considered a false negative in the unmodified test. It is my belief that the test fails to capture the effect of the loss due to settlement recorded in the first place due to multiple reasons, an obvious one is that the users shares will in any case return him more assets than he originally deposited, and this is due to funding payments by core to the vault via the updateFunding function. to be able to properly capture the inflated loss one would need to disable all funding payments in the code. i did this by disabling all update funding calls. if you do that AND disable the settlement Losses, the ethAmount return upon calling redeem on the vault will be equal 1.05 eth, which is original balance deposited by user(1 eth plus the premium of 0.05).

Now, if you enable settlement losses, and keep the invalid formula, the ethAmount rounds to 0.9 ETH, which is 1.05−0.9=0.15 of total loss recorded, which is also reasonable since loss is equal to the entire collateral locked up for option

If we use the correct formula ethAmount += (amount * (strike - getUnderlyingPrice())) / 1e8 , the losses are dramatically less and accurate, ethAmount is 1 ETH. this makes sense because the price moved from 0.15 strike - 0.1 = 0.05.

I hope this clarifies my point.

Tools Used

Manual Review

Recommended Mitigation Steps

first the validation of strike should be as so:

 _validate(strike > getUnderlyingPrice(), 7);

Second the eth amount to be deducted from collateral in vault should be equal to:

ethAmount += (amount * (strike - getUnderlyingPrice())) / 1e8;

Ironically there is already a function that calculates the profit and loss correctly in the perpetual atlantic contract:

  function calculatePnl(
  uint256 price,
  uint256 strike,
  uint256 amount
     ) public pure returns (uint256) {
          return strike > price ? ((strike - price) * amount) / 1e8 : 0;
       }

This function can also be used instead.

Include the fixes mentioned above, but more importantly the entire logic for settlement/redemption needs to be reviewed.

Assessed type

Math