Dopex - bin2chen's results

Lines of code

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

Vulnerability details

Impact

PerpetualAtlanticVaultLP.subtractLoss() Use == to determine if the current contract balance is correct, so that if a malicious user donates 1 wei into the contract will cause this check to always fail, and subtractLoss() will always fail

Proof of Concept

PerpetualAtlanticVaultLP.subtractLoss()Use == to determine if the current contract balance is correct

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

If a malicious user donates 1 wei to the vaultLP contract, this will cause this check to always fail, and subtractLoss() will always fail. It makes sense to just make sure that the current balance is greater than or equal to

tes POC

The following code demonstrates that donating 1 wei to vaultLP will cause the check to always fail, subtractLoss() will always fail.revert Not enough collateral was sent out

add to perp-vault/Unit.t.sol

  function testSettleRevert() public {
    weth.mint(address(1), 1 ether);
    deposit(1 ether, address(1));

    vault.purchase(1 ether, address(this));

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;

    priceOracle.updateRdpxPrice(0.010 gwei); // ITM
    //donate 1 wei to vaultLp
    weth.mint(address(vaultLp), 1 wei);
    // settle will revert
    vault.settle(ids);

  }  

$ forge test --match-test testSettleRevert 

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettleRevert() (gas: 765154)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.22s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettleRevert() (gas: 765154)

Tools Used

Recommended Mitigation Steps

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

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

NFTs transferred to rdpxV2Core.sol using UniV3LiquidityAMO.recoverERC721() will be locked in rdpxV2Core.sol Because rdpxV2Core.sol doesn't have a way to handle NFTs and there is no way to transfer them out again

Proof of Concept

Currently UniV3LiquidityAMO.recoverERC721() defaults to rdpxV2Core.sol.

  function recoverERC721(
    address tokenAddress,
    uint256 token_id
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Only the owner address can ever receive the recovery withdrawal
    // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
      address(this),
@>    rdpxV2Core,
      token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
  }

But in the rdpxV2Core contract, it can accept NFT but there is no way to transfer out or handle NFTs. The only interaction is with RdpxV2Bond, but this is its own internal logic and will not transfer out, and transferring in will not work.

NFTs entering rdpxV2Core will be locked in the contract forever. Suggesting recoverERC721() to transfer the NFT to msg.sender would be a reasonable choice.

Tools Used

Recommended Mitigation Steps

  function recoverERC721(
    address tokenAddress,
    uint256 token_id
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Only the owner address can ever receive the recovery withdrawal
    // INonfungiblePositionManager inherits IERC721 so the latter does not need to be imported
    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
      address(this),
-     rdpxV2Core,
+     msg.sender,
      token_id
    );
    emit RecoveredERC721(tokenAddress, token_id);
  }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

in PerpetualAtlanticVaultLP.deposit() executing perpetualAtlanticVault.updateFunding() after previewDeposit() This results in previewDeposit() not using the latest totalVaultCollateral internally The resulting shares will be larger than the correct value

Proof of Concept

The current deposit() implementation calculates shares first with previewDeposit(), and then executes the perpetualAtlanticVault.updateFunding()

  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);
  }

This results in previewDeposit() not being calculated using the latest totalVaultCollateral and the shares obtained will be greater than the correct value!

Tools Used

Recommended Mitigation Steps

  function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
+   perpetualAtlanticVault.updateFunding();    
    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);
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

miss reduce totalWethDelegated in RdpxV2Core.withdraw() The totalWethDelegated that should have been reduced takes up available weth, causing sync() to throw an error. Dependencies on the sync() method will always revert

Proof of Concept

The current withdraw() does not reduce totalWethDelegated.

  function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

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

@>  //miss reduce totalWethDelegated

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

This results in totalWethDelegated being much larger than it actually is! The weth available for the current contract is subtracted from the totalWethDelegated. This results in the available weth being incorrect and much less. A malicious user could repeatedly addToDelegate() withdraw() to make the number of available weth 0, causing all uses of sync() to underflow.

  function sync() external {
    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;
    }

    emit LogSync();
  }

test POC

The following code demonstrates, after addToDelegate/withdraw ，sync() will underflow

add to Unit.t.sol

  function testWithdrawSyncRevert() public{
    console.log("rdpxV2Core weth balance(before)",weth.balanceOf(address(rdpxV2Core)));
    console.log("rdpxV2Core totalWethDelegated(before)",rdpxV2Core.totalWethDelegated());
    uint256 delegateId = rdpxV2Core.addToDelegate(1e18, 10e8);
    rdpxV2Core.withdraw(delegateId);
    console.log("rdpxV2Core weth balance(after)",weth.balanceOf(address(rdpxV2Core)));
    console.log("rdpxV2Core totalWethDelegated(after)",rdpxV2Core.totalWethDelegated());
    rdpxV2Core.sync();
  }

$ forge test --match-test testWithdrawSyncReve


Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Arithmetic over/underflow] testWithdrawSyncRevert() (gas: 201496)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.69s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Arithmetic over/underflow] testWithdrawSyncRevert() (gas: 201496)

Tools Used

Recommended Mitigation Steps

  function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

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

+   totalWethDelegated -=amountWithdrawn;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

in reLP() , Calculating mintokenAAmount using the wrong formula can invalidate the slippage protection

Proof of Concept

ReLPContract.reLP()The implementation is as follows:

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

@>  mintokenAAmount =
@>    (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
@>    (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    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();
  }

The formula for calculating mintokenAAmount in the above code is ((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8

amountB is the amount of tokenB, but tokenAInfo.tokenAPrice is the price of tokenA, which shouldn't be multiplied together

The correct one should be

((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice)

Tools Used

Recommended Mitigation Steps

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

-    mintokenAAmount =
-     (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
-     (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

+    mintokenAAmount =  ((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) * (1e8 - slippageTolerance) / 1e8;

    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();
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

RdpxV2Core._curveSwap An error in the formula for calculating the minimum amount of minOut for dpxEth may cause the slippage protection to fail.

Proof of Concept

_curveSwap()The code implementation is as follows:

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
    IStableSwap dpxEthCurvePool = IStableSwap(addresses.dpxEthCurvePool);

    // First compute a reverse swapping of dpxETH to ETH to compute the amount of ETH required
    address coin0 = dpxEthCurvePool.coins(0);
    (uint256 a, uint256 b) = coin0 == weth ? (0, 1) : (1, 0);

    // validate the swap for peg functions
    if (validate) {
      uint256 ethBalance = IStableSwap(addresses.dpxEthCurvePool).balances(a);
      uint256 dpxEthBalance = IStableSwap(addresses.dpxEthCurvePool).balances(
        b
      );
      _ethToDpxEth
        ? _validate(
          ethBalance + _amount <= (ethBalance + dpxEthBalance) / 2,
          14
        )
        : _validate(
          dpxEthBalance + _amount <= (ethBalance + dpxEthBalance) / 2,
          14
        );
    }

    // calculate minimum amount out
    uint256 minOut = _ethToDpxEth
@>    ? (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

From the code above we know that if _ethToDpxEth=true uses the formula minOut = (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))

but getDpxEthPrice() is the dpxEth price.

The correct formula should be _amount * 1e8 / getDpxEthPrice()

Tools Used

Recommended Mitigation Steps

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
....

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


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

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

PerpetualAtlanticVault.updateFundingDuration() Does not execute updateFunding() before modifying fundingDuration. may cause the completed epoch to fail to distribute rewards properly and immediately switch to the next epoch.

Proof of Concept

Administrators can modify fundingDuration

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

The current implementation does not perform updateFunding() first to distribute rewards that have already occurred This may cause rewards that have already been completed but not yet counted to be deferred to the next cycle.

It is recommended to execute updateFunding() before setting

Tools Used

Recommended Mitigation Steps

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
+   updateFunding();    
    fundingDuration = _fundingDuration;
  }



## Assessed type

Context

Lines of code

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

Vulnerability details

Impact

UniV2LiquidityAMO.addLiquidity() -> UniV2LiquidityAMO._sendTokensToRdpxV2Core() missing call rdpxV2Code.sync() Causes reserveAsset[].tokenBalance of rdpxV2Code to be inaccurate The bigger problem is that since sync() is not executed, ADMIN can skip checking the balance using the totalWethDelegated balance

Proof of Concept

UniV2LiquidityAMO._sendTokensToRdpxV2Core() lock of call rdpxV2Code.sync()

  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
    );
@>  //miss call rdpxV2Code.sync()
    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

The result is that the tokenBalance in rdpxV2Code is briefly inaccurate and administrators can unrestricted use of the totalWethDelegated

test poc

The following test code demonstrates.

uniV2LiquidityAMO.addLiquidity() illegally uses totalWethDelegated causing the user to be unable to withdraw() properly.

add to Periphery.t.sol

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

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

    // give amo premission to access rdpxV2Core reserve tokens

    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), 50e18);
    uint256 delegateId = rdpxV2Core.addToDelegate(10e18, 1e8);    

    console.log("rdpxV2Core balance (before) :",weth.balanceOf(address(rdpxV2Core)));
    console.log("rdpxV2Core totalWethDelegated (before)",rdpxV2Core.totalWethDelegated());
    // add liquidity
    uniV2LiquidityAMO.addLiquidity(50e18, 10e18, 0, 0);
    console.log("rdpxV2Core balance (after) :",weth.balanceOf(address(rdpxV2Core)));
    console.log("rdpxV2Core totalWethDelegated (after)",rdpxV2Core.totalWethDelegated());
  
    vm.expectRevert(bytes("ERC20: transfer amount exceeds balance"));
    rdpxV2Core.withdraw(delegateId);

  }

$ forge test -vvv --match-test testUseDelegateBalance

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testUseDelegateBalance() (gas: 2294701)
Logs:
  rdpxV2Core balance (before) : 10000000000000000000
  rdpxV2Core totalWethDelegated (before) 10000000000000000000
  rdpxV2Core balance (after) : 0
  rdpxV2Core totalWethDelegated (after) 10000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.99s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Recommended Mitigation Steps

  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
    );
+   addresses.rdpxV2Core.sync()
    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

Assessed type

Context