Dopex - asui's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L764-L784

Vulnerability details

Impact

Options can never be settled.

Proof of Concept

When the admin calls the settle function in the RdpxV2Core.sol the steps are:

function settle(
    uint256[] memory optionIds
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 amountOfWeth, uint256 rdpxAmount)
  {
    _whenNotPaused();
    (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).settle(optionIds);
    for (uint256 i = 0; i < optionIds.length; i++) {
      optionsOwned[optionIds[i]] = false;
    }

    reserveAsset[reservesIndex["WETH"]].tokenBalance += amountOfWeth;
    reserveAsset[reservesIndex["RDPX"]].tokenBalance -= rdpxAmount;

    emit LogSettle(optionIds);
  }

we can see the line

(amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
     addresses.perpetualAtlanticVault
   ).settle(optionIds);

if we further look at the .settle(optionIds) in the perpetualAtlanticVault.sol we can see: that it calls IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(ethAmount)

Keep in mind the ethAmount calculation is based on existing values and not collateral.BalanceOf(perpetualAtlanticVaultLP) which makes the attack possible.

If we again look into this .subtractLoss(ethAmount) function in the perpetualAtlanticVaultLP.sol contract :

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

we can see the check line collateral.balanceOf(address(this)) == _totalCollateral - loss. The problem here is that if someone directly sends some collateral token(i.e. weth) to the perpetualAtlanticVaultLP.sol this check will always fail which results in permanent DOS. It can also be attacked by frontrunning the the transaction and providing dust amount of collateral. The frontrunning could've been a temporary DOS but since the contract doesn't have functions to deal with extra collateral tokens either way the function is in the state of a permanent DOS.

Tools Used

manual review

Recommended Mitigation Steps

use proper functions to deal with external collateral tokens received. Remember that the function can also be attacked by a frontrunner by providing dust amount of collateral to the perpetualAtlanticVaultLP.sol address whenever the settle function is called by the admin. So consider adding a sync function type which will be called inside the subtractLoss function before the check.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

weth assets locked in the contract. And can impact the core functionalities of the protocol.

Proof of Concept

In the RdpxV2Core.sol contract, when users delegate their weth by calling the addToDelegate function the totalWethDelegated count is also increased totalWethDelegated += _amount; however when users withdraw their delegate the count is not decreased:

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;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

so far so good. But when someone calls the sync function:

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

we can see that when the token is weth it separates the balance from the totalWethDelegated. So after the users withdraw their delegates, when the sync is called some amount of weth is always deducted from the reserve of weth.

Or worse, an attacker could come in with a huge amount of weth(almost the same as the protocol's weth amount) and calls the addToDelegate function and immediately withdraw before anyone could use his weth just to increase the totalWethDelegated count. He can also do this by creating a contact that calls addToDelegate and withdraw at one transaction. And finally calls the sync function to reduce(or lock) the reserves of weth. Even though the weth is not directly locked since the contract uses reserveAsset[reservesIndex["WETH"]].tokenBalance this everytime the contract still has the weth but cannot use it which results in the failure of the system.

Since the attacker can call delegate withdraw multiple times and the totalWethDelegated is not reduced in the withdraw function some point the totalWethDelegated will be greater than the reserveAsset[reservesIndex["WETH"]].tokenBalance so the sync function will be in a DoS state and can affect the sync of other tokens too.

Tools Used

manual review

Recommended Mitigation Steps

add totalWethDelegated -= amountWithdrawn; in the withdraw function.

Assessed type

Other

1. call the sync() function every time an asset is sent from one contract to the RdpxV2Core contract. Do this by adding RdpxV2Core.sync() in the functions that transfers or pulls assets from the RdpxV2Core contract. For example in the swap functions in UniV2LiquidityAMO and UniV3LiquidityAMO contracts. The sync function can be called anytime but very large amount of swaps can cause calculation issues if no one called the sync function in a very long time but this is very unlikely to happen.

2. There are many functions where the admin could rug pull users or send tokens to another accounts:

   • function approveContractToSpend and function emergencyWithdraw in the UniV2LiquidityAmo.sol contract.
   • function approveTarget in the UniV3LiquidityAmo.sol contract.
   • function emergencyWithdraw in the RdpxDecayingBonds.sol contract.
   • function emergencyWithdraw and function approveContractToSpend in the RdpxV2Core.sol contract.

Users of the protocol should be made aware of these admin controlled functions.

3. In the swap function in the UniV3LiquidityAmo.sol contract the admin can swap any tokenA to tokenB while the tokenA has to be taken from the core contract the tokenB can be any other type of token so the admin can swap any token and this token will be sent back to the core contract. So add proper functions to deal with these tokens or restrict the tokens to be swap to only weth and rdpx like we did in the UniV2LiquidityAMO.sol contract. There is also no restriction for the admin to swap so he can swap how much time he likes.

4. The RdpxV2Core.sol contract highly depends on the default admin role consider using multisig owner. Owner can also rug pull users by calling the pause function and then withdrawing all the funds. OR if the private key of the defaultAdminRole is compromised or is lost the protocol will face many issues. Admin also has the privilege to add or remove addresses like pricingOracleAddresses, AMOAddresses, etc.

5. In the RdpxV2Core.sol contract the admin can call function setRdpxBurnPercentage and function setRdpxFeePercentage to change the rdpxBurnPercentage and rdpxFeePercentage respectively but if they forget to add the DEFAULT_PRECISION which is e18 or miscalculate it, it can cause many issues in the calculations. Consider adding the DEFAULT_PRECISION inside the function which will be multiplied by the admin input value if possible.

6. In the function setBondMaturity in RdpxV2Core.sol contract the admin can set the bondMaturity to any value he likes as long as it is greater than 0. Admin can set it to uint256 max or a very large value and those users who bond after this is set cannot redeem their bonds. Consider adding a check in the setBondMaturity function so that the admin cannot set the bondMaturity to more than a restricted value. Or let the users be aware of this.

7. In the function removeAssetFromtokenReserves in the RdpxV2Core.sol contract the admin has the privilege to remove any token from the reserve. If the admin removes tokens like weth this can cause many issues. Users should be made aware of this. OR restrict the admin from removing core tokens like weth and rdpx atleast.

8. In the RdpxV2Core.sol contract the admin can call the upperDepeg or the lowerDepeg function to achieve the pegged of rdpx and eth 1:1 ratio but both this functions doesnt check that after they are called and the swap is done it doesn't check that it is actually pegged in the desired ratio. Admin could pass a wrong value and instead of pegging it to 1:1 he could mistakenly or intentionally further the 1:1 desired pegged. like In the upperDepeg function there is this check _validate(getDpxEthPrice() > 1e8, 10) so as long as this check pass admin could pass a very large amount and further make the ratio worse.

9. Wrong error code passed in the calculateBondCost function in the RdpxV2Core.sol contract.

10. The constructor in the UniV3LiquidityAMO.sol contract has hard coded address values. Avoid these if possible.

11. In the UniV3LiquidityAmo.sol contract in the function execute the function will pass even if the call fails. Try adding require(success) so that the transaction will revert if the call fails.

12. In the convertToShares in PerpetualAtlanticVaultLp.sol contract return value returns (uint256 shares) is declared but not used. Either use it or remove it.

13. import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; this import in the RdpxV2Bond contract is not needed because the ERC721Enumerable already imports it.