Dopex - ladboy233's results

Lines of code

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

Vulnerability details

Impact

AtlanticVault.sol#settle can be DOSed when admin want to settle with loss

Proof of Concept

In function settle PerpetualAtlanticVault

the line of code is called below

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

which calls subractLoss in VaultLp contract

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

noteo the line of code:

   require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

this check is too strict, before the AtlanticVault.sol#settle called, as long as user transfer 1 wei of collateral to the AtlanticVaultLP contract

the check above revert and break, and then the admin cannot settle the option id on time

Tools Used

Manual Review

Recommended Mitigation Steps

do not use strict comparision

  collateral.balanceOf(address(this)) == _totalCollateral - loss

can change to >= instead of ==

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L361 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L873 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L964

Vulnerability details

Impact

Bond / BondWithDelegate / amo / relp can get consistently DOSed because totalWethDelegated is not updated when delegate withdraw WETH

Proof of Concept

the bond / bondWithDelegate / amo / relp are all core operation

In UniswapV3LiquidityAmo when admin swap / add liquidity and mint position NFT / decrease liquidity

the function _sendTokensToRdpxV2Core is called

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

and the function call sync()

IRdpxV2Core(rdpxV2Core).sync();

when bond / bondWithDelegate, and the relp flag turns on,

this code will execute

 // reLP
    if (isReLPActive) IReLP(addresses.reLPContract).reLP(totalBondAmount);

according to the docs:

Finally a reLP process is applied on the Uniswap V2 liquidity added by the Uniswap V2 AMO. This is toggleable and hence may or may not be used on every bond execution depending on the market conditions of rDPX.

In the end of the relp function, sync() is called as well to sync the reserve balance of WETH and RDPX

However, the cost of DOS the functon sync is cheap

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

how to make this function consistently revert?

note this line of code:

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

the totalWethDelegated is updated in two other places,

when user call addToDelegate, totalWethDelegated is increased

// add amount to total weth delegated
totalWethDelegated += _amount;

when user bondWithDelegate, the totalWethDelegated is decreased

_validate(delegate.amount - delegate.activeCollateral >= wethRequired, 5);
delegate.activeCollateral += wethRequired;

// update total weth delegated
totalWethDelegated -= wethRequired;

the user can addToDelegate and set a fee and delegate WETH, so user with only rdpx can pair with the delegated WETH to complete the bond operation

the user can withdraw the not-used WETH by calling RdpxV2Core.sol#withdraw

However, when the delegator withdraw not-used WETH in this way, the totalWethDelegated is not decreased and therefore the totalWethDelegated is inflated

then it is very obvious that

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

can revert in underflow

suppose WETH balance is 0 ETH

user addToDelegate with 0.5 WETH, now the contract hold 0.5 WETH,

the totalWethDelegated is 0.5 WETH

then user can withdraw, the contract balance hold 0 WETh

but totalWethDelegated is still 0.5 WETh

then balance - totalWethDelegated -> 0 WETH - 0.5 WETH -> underflow

basically all a malicious actor needs to do is call addToDelegate and then withdraw to inflate the totalWethDelegated state

then sync would revert in underflow, and relp revert in underflow when calling sync, and if the relp flag turns on, the bond operation revert as well

the POC below shows that addToDelegate then withdraw can make sync revert in underflow

In tests\rdpxV2-core\Unit.t.sol,

add

  function testDelegate_POC() public {
     uint256 userBalance = weth.balanceOf(address(this));
     uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);
     rdpxV2Core.withdraw(delegateId);
     rdpxV2Core.sync();
  }

then run the test

and we can see that transaction revert in underflow when calling sync

PS D:\2023Security\2023-08-dopex> forge test --match testDelegate_POC
[⠔] Compiling...
[⠆] Compiling 1 files with 0.8.19
[⠒] Solc 0.8.19 finished in 11.85s
Compiler run successful (with warnings)
warning[2072]: Warning: Unused local variable.
   --> tests/rdpxV2-core/Unit.t.sol:103:6:
    |
103 |      uint256 userBalance = weth.balanceOf(address(this));
    |      ^^^^^^^^^^^^^^^^^^^




Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Arithmetic over/underflow] testDelegate_POC() (gas: 192797)
Test result: FAILED. 0 passed; 1 failed; finished in 149.54ms

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

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review,

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;
	
	  // update totalWethDelegated state here
	  totalWethDelegated -= amountWithdrawn;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

AddLiquidity during Relp missing slippage protection

Proof of Concept

According to the docs:

https://dopex.notion.site/rDPX-V2-RI-b45b5b402af54bcab758d62fb7c69cb4

reLP is the process by which part of the Uniswap V2 liquidity is removed, then the ETH received is swapped to rDPX.

when adding liquidity, the min token out of A and B are set to 0

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

the protocol intended to deploy on arbitrum, the sequencer is in charge of ordering transaction

even it is difficult to frontrun and sandwich the addLiquidity like other network,

the user cannot make sure there is no other transaction that change the liquidity of Uniswap V2 pool landed before the relp transaction in the same block

so a very suboptimal amount of Lp can be minted

Tools Used

Manual Review

Recommended Mitigation Steps

does hardcode amountAMin and amountBMin to 0

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

Bond missing slippage control and can cost too much WETH of caller

Proof of Concept

When bond, user pays WETH and RDPX togther

  /**
   * @notice The primary bonding function
   * @param  _amount The amount of DpxEth to bond for
   * @param  rdpxBondId The bond id
   * @param  _to The address to send the bond to
   * @return receiptTokenAmount the amount of receipt tokens received
   **/
  function bond(
    uint256 _amount,
    uint256 rdpxBondId,
    address _to
  ) public returns (uint256 receiptTokenAmount) {

the exact amount of the charged WETH and RDPX is calculated by the function

// Compute the bond cost
(uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
  _amount,
  rdpxBondId
);

If we take a look at the logic inside calculateBondCost, the amout of rdpxRequired

depends on

1. the input parameter amount which can be controlled by the user
2. the rdpxPrice, which user has no control

  // if rdpx decaying bonds are being used there is no discount
  rdpxRequired =
	(RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) /
	(DEFAULT_PRECISION * rdpxPrice * 1e2);

  wethRequired =
	(ETH_RATIO_PERCENTAGE * _amount) /
	(DEFAULT_PRECISION * 1e2);

In case there is a price strike for rdpxPrice, the user can be charged too much rdpx when bonding

the amount of the WETH required depends on

1. the input parameter amount which can be controlled by the user
2. the premium amount which depends on the strike price, which depends on the user-no-control rdpx price as well

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

uint256 timeToExpiry = IPerpetualAtlanticVault(
  addresses.perpetualAtlanticVault
).nextFundingPaymentTimestamp() - block.timestamp;
// @audit
if (putOptionsRequired) {
  wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
	.calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
}

3. the premium user has minimal control, the premium depends on the time to expiration and the volativilty and the the rpdx underlying price if we take a look at the calculatePremium logic

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
      timeToExpiry
    ) * _amount) / 1e8);
  }

in the case when there is a volatile period or there is a price strike of rdpx price, too much premium can be charged in the form of WETH from caller

then the amount of too much WETH is transfered from user in this line of code

the amount of too much RDPX is transfered from user in this line of code

suppose the admin submit a transaction to update the oracle price first, while the sequencer is finalizing the transaction

a user can call bond, but again as mentioned above, combining with the fact that the bond function lacks slippage control, suddenly price spike of the rdpx price can cause user get charged too much WETH and RDPX when bonding

Tools Used

Manual Review

Recommended Mitigation Steps

consider implement slippage control when user bonding and limit the max WETH used and max RDPX uesd

Assessed type

Timing

Lines of code

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/amo/UniV2LiquidityAmo.sol#L160 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L486 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1135

Vulnerability details

Impact

UniV2LiquidityAmo.sol does not sync core contract balance correctly and break the Core contract accounting

Proof of Concept

In RdpxV2Core.sol contract

the code use reserveAssets to track WETH token and RDPX token balance

which is

reserveAsset[reservesIndex["WETH"]].tokenBalance

and

reserveAsset[reservesIndex["RDPX"]].tokenBalance

this is crucial to the Core contract accounting

In UniV3LiquidityAmo.sol,

whenever calling addLiquidity or removeLiqudity or swap,

the function _sendTokensToRdpxV2Core is called, this is the code

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

note the function call

IRdpxV2Core(rdpxV2Core).sync();

and the function sync call sync the balance of reserved token properly

this function update the reservesAsset token balance correctly in core contract

However, in UniV2LiquidityAmo.sol#_sendTokensToRdpxV2Core, the line of code

IRdpxV2Core(rdpxV2Core).sync();

is never called

but the tokenA and tokenB are sent to Core contract, so without calling sync in Core contract, the

reserveAsset[reservesIndex["WETH"]].tokenBalance

and

reserveAsset[reservesIndex["RDPX"]].tokenBalance

under-estimate the asset balance

this becomes a issue when the the core contract tries to update the token balance, especially when the contract tries to substract from the balance

reserveAsset[reservesIndex["RDPX"]].tokenBalance -= _rdpxAmount;

and

 reserveAsset[reservesIndex["WETH"]].tokenBalance -= _wethAmount;

one of the example is when the code tries to purchase the option

// update weth reserve
reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

// purchase options
uint256 premium;
if (putOptionsRequired) {
  premium = _purchaseOptions(rdpxRequired);
}

and

  /**
   * @notice Purchase rdpx put options
   * @param _amount amount of rdpx to purchase
   * @return premium amount of premium paid
   */
  function _purchaseOptions(
    uint256 _amount
  ) internal returns (uint256 premium) {
    /**
     * Purchase options and store ERC721 option id
     * Note that the amount of options purchased is the amount of rDPX received
     * from the user to sufficiently collateralize the underlying DpxEth stored in the bond
     **/
    uint256 optionId;

    (premium, optionId) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).purchase(_amount, address(this));

    optionsOwned[optionId] = true;
    reserveAsset[reservesIndex["WETH"]].tokenBalance -= premium;
  }

the code tries to purchase put option and the premium is subtract from the WETH balance

suppose reserveAsset[reservesIndex["WETH"]].tokenBalance is 1 WETH

then UniV2LiquidityAmo.sol#addLiquidity is triggered and _sendTokensToRdpxV2Core add 0.5 WETH to the core contract

the real balance of WETH 1.5 WETH, while the contract think it only has 1 WETH

suppose user tries to bond and transfer 1 WETH in this line of code,

// update weth reserve
reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

now reserveAsset[reservesIndex["WETH"]].tokenBalance is 2 WETH (in reality the contract 2.5 WETH)

and then premium that needs to pay is 2.1 WETH

2 WETH - 2.1 WETH in this line of code can revert in underflow

  reserveAsset[reservesIndex["WETH"]].tokenBalance -= premium;

while in reality the contract hold 2.5 WETH (2 WETH original WETH + 0.5 WETH from UniV2LiquidityAmo.sol#addLiquidity) and user should be able to purchase the option

but because the sync is never called, the update of balance of WETH and RDPX is delayed in the core contract

further more, the core contract exposes a view function, without sync the core contract balance properly, the external integration that replies on the api getReserveTokenInfo can retrieve less token balance than it should be

and the function getReserveTokenInfo is no longer a reliable source for external integration to retrieve the token balance state.

  function getReserveTokenInfo(
    string memory _token
  ) public view returns (address, uint256, string memory) {
    ReserveAsset memory asset = reserveAsset[reservesIndex[_token]];

    _validate(
      keccak256(abi.encodePacked(asset.tokenSymbol)) ==
        keccak256(abi.encodePacked(_token)),
      19
    );

    return (asset.tokenAddress, asset.tokenBalance, asset.tokenSymbol);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Sync core contract balane properly after changing the core contract token balance

Assessed type

Token-Transfer

Lines of code

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/amo/UniV3LiquidityAmo.sol#L119

Vulnerability details

Impact

UniV3LiquidityAmo.sol does not sync core contract balance correctly when collecting fee and break the Core contract accounting

Proof of Concept

In RdpxV2Core.sol contract

the code use reserveAssets to track WETH token and RDPX token balance

which is

reserveAsset[reservesIndex["WETH"]].tokenBalance

and

reserveAsset[reservesIndex["RDPX"]].tokenBalance

this is crucial to the Core contract accounting

In UniV3LiquidityAmo.sol,

whenever calling addLiquidity or removeLiqudity or swap,

the function _sendTokensToRdpxV2Core is called, this is the code

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

note the function call

IRdpxV2Core(rdpxV2Core).sync();

and the function sync call sync the balance of reserved token properly

this function update the reservesAsset token balance correctly in core contract

However, in UniV3LiquidityAmo.sol#collectFees, the line of code

IRdpxV2Core(rdpxV2Core).sync();

is never called

  function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
    for (uint i = 0; i < positions_array.length; i++) {
      Position memory current_position = positions_array[i];
      INonfungiblePositionManager.CollectParams
        memory collect_params = INonfungiblePositionManager.CollectParams(
          current_position.token_id,
          rdpxV2Core,
          type(uint128).max,
          type(uint128).max
        );

      // Send to custodian address
      univ3_positions.collect(collect_params);
    }
  }

but the tokenA and tokenB from the LP fee on Uniswap V3 are sent to Core contract, so without calling sync in Core contract, the

reserveAsset[reservesIndex["WETH"]].tokenBalance

and

reserveAsset[reservesIndex["RDPX"]].tokenBalance

under-estimate the asset balance

this becomes a issue when the the core contract tries to update the token balance, especially when the contract tries to substract from the balance

reserveAsset[reservesIndex["RDPX"]].tokenBalance -= _rdpxAmount;

and

 reserveAsset[reservesIndex["WETH"]].tokenBalance -= _wethAmount;

one of the example is when the code tries to purchase the option

// update weth reserve
reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

// purchase options
uint256 premium;
if (putOptionsRequired) {
  premium = _purchaseOptions(rdpxRequired);
}

and

  /**
   * @notice Purchase rdpx put options
   * @param _amount amount of rdpx to purchase
   * @return premium amount of premium paid
   */
  function _purchaseOptions(
    uint256 _amount
  ) internal returns (uint256 premium) {
    /**
     * Purchase options and store ERC721 option id
     * Note that the amount of options purchased is the amount of rDPX received
     * from the user to sufficiently collateralize the underlying DpxEth stored in the bond
     **/
    uint256 optionId;

    (premium, optionId) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).purchase(_amount, address(this));

    optionsOwned[optionId] = true;
    reserveAsset[reservesIndex["WETH"]].tokenBalance -= premium;
  }

the code tries to purchase put option and the premium is subtract from the WETH balance

suppose reserveAsset[reservesIndex["WETH"]].tokenBalance is 1 WETH

then UniV2LiquidityAmo.sol#addLiquidity is triggered and _sendTokensToRdpxV2Core add 0.5 WETH to the core contract

the real balance of WETH 1.5 WETH, while the contract think it only has 1 WETH

suppose user tries to bond and transfer 1 WETH in this line of code,

// update weth reserve
reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

now reserveAsset[reservesIndex["WETH"]].tokenBalance is 2 WETH (in reality the contract 2.5 WETH)

and then premium that needs to pay is 2.1 WETH

2 WETH - 2.1 WETH in this line of code can revert in underflow

  reserveAsset[reservesIndex["WETH"]].tokenBalance -= premium;

while in reality the contract hold 2.5 WETH (2 WETH original WETH + 0.5 WETH from UniV2LiquidityAmo.sol#addLiquidity) and user should be able to purchase the option

but because the sync is never called, the update of balance of WETH and RDPX is delayed in the core contract

further more, the core contract exposes a view function, without sync the core contract balance properly, the external integration that replies on the api getReserveTokenInfo can retrieve less token balance than it should be

and the function getReserveTokenInfo is no longer a reliable source for external integration to retrieve the token balance state.

  function getReserveTokenInfo(
    string memory _token
  ) public view returns (address, uint256, string memory) {
    ReserveAsset memory asset = reserveAsset[reservesIndex[_token]];

    _validate(
      keccak256(abi.encodePacked(asset.tokenSymbol)) ==
        keccak256(abi.encodePacked(_token)),
      19
    );

    return (asset.tokenAddress, asset.tokenBalance, asset.tokenSymbol);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Sync core contract balane properly after changing the core contract token balance

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

RDPX reserve balance can be inflated to apply too much discount when bond or cause too much LP to remove when relp

Proof of Concept

while the reserve logic is out of scope, the flaw clearly impact the in-scope code

when Relp, the logic of calculating how much discount depends on the spot reserve balance of rdpx

  function calculateBondCost(
    uint256 _amount,
    uint256 _rdpxBondId
  ) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
    uint256 rdpxPrice = getRdpxPrice();

    if (_rdpxBondId == 0) {
      // @audit
      uint256 bondDiscount = (bondDiscountFactor *
        Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
        1e2) / (Math.sqrt(1e18)); // 1e8 precision

      _validate(bondDiscount < 100e8, 14);

      rdpxRequired =
        ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) *
          _amount *
          DEFAULT_PRECISION) /
        (DEFAULT_PRECISION * rdpxPrice * 1e2);

      wethRequired =
        ((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /
        (DEFAULT_PRECISION * 1e2);
    } 

a user can inflate the rdpxReserve by sending the rdpx directly to reserve balance

then the bondDiscount applied is larger and user can pay less rdpx and weth when bonding

the POC is here in tests\rdpxV2-core\Unit.t.sol

  function testTooMuchDiscount_Bond_POC() public {
  
      uint256 userwethBalance = weth.balanceOf(address(this));
      uint256 userRdpxBalance = rdpx.balanceOf(address(this));

      (uint256 rdpxRequired, uint256 wethRequired) = rdpxV2Core.calculateBondCost(
        1 * 1e18,
        0
      );

      console.log("rdpx required", rdpxRequired);
      console.log("weth required", wethRequired);

      address reserve = address(rdpxReserveContract);

      rdpx.transfer(reserve, 10000 ether);

      console.log("amount of WETH and RDPX to bond after applying discount");

      (uint256 rdpxRequiredAfter, uint256 wethRequiredAfter) = rdpxV2Core.calculateBondCost(
        1 * 1e18,
        0
      );

      console.log("rdpx required", rdpxRequiredAfter);
      console.log("weth required", wethRequiredAfter);
    
  }

we run the POC and we get:

rdpx required 1225000000000000000
weth required 806250000000000000
amount of WETH and RDPX to bond after applying discount
rdpx required 998753109500000000
weth required 749688277375000000

after we inflate the reserved balance of the reserve contract,

the amount of rdpx and weth to bond is much smaller,

note, wether this is profitable for attack depends on whether the attack can cash out the minted bond in external NFT market

and also depending on the price of rdpx and weth, in the case when rdpx is very cheap while ETH is expensive, the attacker can buy more rdpx to inflate the reserve balance and pay less WETH to get higher discount

Tools Used

Manual Review

Recommended Mitigation Steps

do not use the consume the spot balance of the rdpx reserve balance when calcuting bonding cost

Assessed type

Other