Dopex - HHK's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L324

Vulnerability details

Impact

Calling recoverERC721() on the UniV3 AMO will lock the position indefinitely.

Because the position NFT is sent to the core contract but the core contract doesn't have any function to allow transfer of such NFT and or burn it. Resulting in the liquidity being lock forever.

Proof of Concept

In recoverERC721() the NFT is sent to the core contract on line 330.

But in the core contract there is no function to interact with.

While the function is only callable by the owner, it seems unlikely that the idea behind it is to lock the NFT and liquidity forever.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a function to transfer back the NFT on the core contract or send the NFT to the msg.sender or a toaddress when calling recoverERC721().

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

When calling deposit() the shares to be received are calculated before calling updateFunding() which result in the new depositor earning funding even tho it just deposited, at the expense of older depositors.

Proof of Concept

In the deposit() function, the shares to be received are calculated by calling previewDeposit() which doesn't account for the funding to be received.

So then the function calls perpetualAtlanticVault.updateFunding() right after which will result in the shares value to increase as some WETH will be sent from the vault contract to the LP contract.

A new depositor will instantly receive the rewards like other depositors since the last updateFunding() call even tho it just deposited.

One could monitor funding to be sent and deposit() then redeem() every time it's worth the gas price resulting in no increase of the LP liquidity and less rewards for actual liquidity providers.

Simple POC:

This is essentially the same as testFunding() but we assert that new depositor would get more shares if depositing before than after updateFunding().

Can be copy pasted in tests/perp-vault/Unit.t.sol:Unit.

function testFundingSharesIncrease() public {
        // mint 1 weth to address 1, address 1 LPs 1 weth, rdpxV2Core purchases 1 option, update price to <strike, calculate funding
        weth.mint(address(1), 1 ether);
        deposit(1 ether, address(1));

        vault.purchase(1 ether, address(this));
        priceOracle.updateRdpxPrice(0.014 gwei);
        skip(86500); // skip to expiry
        uint256[] memory strikes = new uint256[](1);
        strikes[0] = 0.015 gwei;

        //check share price before paying funding
        uint sharesBefore = vaultLp.previewDeposit(1 ether);

        vault.payFunding();
        vault.updateFunding();

        //check share price after paying funding
        uint sharesAfter = vaultLp.previewDeposit(1 ether);

        //Assert that depositing before would give user more shares
        assertGt(sharesBefore, sharesAfter);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Call updateFunding() first in the deposit() function so share value is updated before being calculated.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L975 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L995 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L941

Vulnerability details

Impact

totalWethDelegated is not updated on withdraw() but is being used to compute the WETH balance of the core contract when calling sync().

This can result in the internal balance being less than the real balance and revert when the WETH are used and substracted by different functions of the core contract.

Proof of Concept

When calling addToDelegate the function adds the WETH deposited to totalWethDelegated but not to the internal balance as these WETH are not technically available for the core contract, they're waiting for another user to pair RDPX with.

Thus when we call sync() the totalWethDelegated is substracted from the WETH balance to limit the core contract to only WETH that have actually been used for bonding.

But because it is not updated in withdraw the next time someone call sync(), the internal balance will be smaller than it actually is.

Having a smaller balance will impact the abilities of the protocol to use different functions such as:

• lowerDepeg() could revert as the _wethAmount substracted might be bigger than the internal balance on line 1110.
• provideFunding() could revert as the funding amount could be bigger than the internal balance on line 803.
• sync() itself as with time or if an attacker keep delegating then withdrawing the totalWethDelegated could become bigger than the WETH balance itself and revert on line 1002. This would impact the Uniswapv3 AMO as the function _sendTokensToRdpxV2Core() relies on it, as well as the reLP contract in the function reLP() it calls sync()at the end. Having the relp() revert will cause bonding to revert if the core contract has the isReLPActive variable set to true.

POC:

We show how sync() would revert if we delegate then withdraw WETH so the totalWethDelegated cause an underflow.

This test can be copy pasted in tests/rdpxV2-core/Unit.t.sol:Unit.

 function testTotalWethDelegated() public {
      //delegate tokens to the core contract
      uint id = rdpxV2Core.addToDelegate(10 ether, 5e8);

      //totalWethDelegated was updated
      assertEq(rdpxV2Core.totalWethDelegated(), 10 ether);

      //we can call sync since 10 - 10 = 0 so no revert
      rdpxV2Core.sync();

      //Check WETH internal balance is still 0
      (, uint balance, string memory symbol) = rdpxV2Core.reserveAsset(2);
      assertEq(symbol, "WETH");
      assertEq(balance, 0);

      //now we withdraw our WETH
      rdpxV2Core.withdraw(id);

      //totalWethDelegated was NOT updated
      assertEq(rdpxV2Core.totalWethDelegated(), 10 ether);

      //If we try to call sync it reverts with underflow
      vm.expectRevert(stdError.arithmeticError);
      rdpxV2Core.sync();
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Update the totalWethDelegated in withdraw().

amountWithdrawn = delegate.amount - delegate.activeCollateral;
_validate(amountWithdrawn > 0, 15);
delegate.amount = delegate.activeCollateral;

totalWethDelegated -= amountWithdrawn;

IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

Assessed type

Error

Judge has assessed an item in Issue #2142 as 2 risk. The relevant finding follows:

LOW1: No _whenNotPaused() in redeem() Technical Details Almost all state changing functions have _whenNotPaused() in the core contract but it is not the case for redeem().

The NFT it interact with has a pause/unpause functionnality so the function will revert if the NFT is paused thus the impact seems low.

Impact User could still redeem even tho the core contract is paused.

Recommendation Add _whenNotPaused() to the function.

GAS1: reserveTokens is not needed

Technical Details

In the core contract, the storage array reserveTokens is not needed as the token symbols are being stored in the reserveAsset array.

Recommendation

Remove reserveTokens and update the function removeAssetFromtokenReserves() to use the reserveAsset array.

GAS2: If minAmount is set then no need to compute minOut.

Technical Details

In the function _curveSwap() of the core contract. minOut is always computed although it won't be use if minAmount was set in the params.

Recommendation

Add an if statement to not compute minOut if minAmount > 0.

GAS3: Simplify _calculateAmounts() ratio

Technical Details

In the function _calculateAmounts() of the core contract, the amount1 is calculated by recomputing the rdpx value and then finding the ratio of the bond amount.

But because the ratio of ETH and RDPX is a constant 75/25, we could just take 25% of the amount (rdpx part) and then apply the delegate fee.

Recommendation

Remove the oracle call and replace the amount1 calculation with:

// amount required for delegatee
amount1 = _amount * RDPX_RATIO_PERCENTAGE / (100 * DEFAULT_PRECISION)
// account for delegate fee
amount1 = (amount1 * (100e8 - _delegateFee)) / 1e10;

GAS4: _whenNotPaused() not needed in mint()

Technical Details

In the mint() function of the rDpxDecayingBond, the check _whenNotPaused() is not needed as the internal mint function will call beforeTokenTransfer()which has this check already.

Recommendation

Remove _whenNotPaused() from the function.

GAS5: Save array index instead of Position struct

Technical Details

In the UniV3 AMO contract there is positions_array and a positions_mapping. Because the mapping is only used to link a tokenId to a Position we could save gas by just saving the index of the positions_array as a simple mapping(uint256 => uint256).

Recommendation

Use a mapping(uint256 => uint256) instead of mapping(uint256 => Position).

GAS6: roundingPrecision could be constant

Technical Details

In the PerpetualVault contract the variable roundingPrecision could be constant as it's never updated.

Recommendation

Set the variable to constant.

GAS7: Pass currentPrice to calculatePremium() instead of 0

Technical Details

In the function purchase() of the perpetualVault, the call to calculatePremium() could cost less gas by passing the variable currentPrice instead of 0 as this will force the function to call the oracle again.

Recommendation

Pass currentPriceinstead of 0.

GAS8: Remove positionId from the OptionPosition struct.

Technical Details

In the perpetualVault, the OptionPosition struct saves the optionId.

But because it is only used with a mapping that already store the id as key, saving it in the struct is useless.

Recommendation

Remove positionId from the OptionPosition struct.