Dopex - codegpt's results

Lines of code

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

Vulnerability details

Impact

An attacker is able to launch a Denial of Service attack on the settle() function of the PerpetualAtlanticVault.sol contract.

Proof of Concept

The settle will trigger the subtractLoss() function of the perpetualAtlanticVaultLP contract, which contains a require statement that requires the balance of the collateral in the perpetualAtlanticVaultLP contract to be equal to _totalCollateral - loss.

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

However, the attacker is able to launch a Denial of Service attack by directly sending some WETH (the collateral tokens) to the contract and and deliberately inflating the value of collateral.balanceOf(address(this)), thus causing the require statement always revert.

Test

 function testSettleDoS() 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.2 gwei); // initial price * 10
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;
    skip(86500); // expire

    // DoS Settle by sending some weth to the vault LP contract
    mintWeth(1 ether, address(vaultLp));

    vault.settle(ids);

  }

The test case will always revert and return the following message:

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

Tools Used

Recommended Mitigation Steps

Recommend reconsidering the restriction of the require statement and changing it properly.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When transferring tokens A and B to rdpxV2Core using the _sendTokensToRdpxV2Core() method in Uniswap V2 AMO contract, the token balances are not synchronized through IRdpxV2Core(rdpxV2Core).sync();. This can lead to discrepancies between the token reserves in rdpxV2Core and the actual amounts held.

Proof of Concept

The _sendTokensToRdpxV2Core() in Uniswap V2 AMO is missing rdpxV2Core.sync(), however the sync has been done in Uniswap V3 AMO.

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

    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

Tools Used

Manual

Recommended Mitigation Steps

Recommend adding token sync invocation to avoid data inconsistency:

    // sync token balances
    IRdpxV2Core(rdpxV2Core).sync();

Assessed type

Other

Missing Validation on upper bound

Code Position

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

Description

The following setter functions are missing upper bounds, which may cause unexpected functionalities:

1. Both RdpxBurnPercentage and RdpxFeePercentage should be less than 100 * precision and the sum of these two values should not be larger than 100 * precision. Exceeded value may cause error in _transfer function.
2. slippageTolerance should be less than 100 * precision.

Recommendation

Recommend adding upperbound when setting values above to avoid unexpected functionalities.

Unused slippage variable

Code Position

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L109C1-L117C4

Description

The slippageTolerance value is not being used in the UniV2LiquidityAMO contract, as a result, the slippage control relies on the function input, for example, token2AmountOutMin in swap function.

  function setSlippageTolerance(
    uint256 _slippageTolerance
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(
      _slippageTolerance > 0,
      "reLPContract: slippage tolerance must be greater than 0"
    );
    slippageTolerance = _slippageTolerance;
  }

Recommendation

Recommend removing unused contract component, or complete functionality with slippage control.

Missing Validation on Duplicate AMO address

Code Position

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

Description

The privileged function addAMOAddress is being used to update the amoAddresses list by DEFAULT_ADMIN_ROLE.

However, there is no limitation on the duplicate address when adding a new AMO address.

Recommendation

It is recommend to adding duplicate filter when adding new AMO addresses.

Unused Contract Component

Code Position

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

Description

The following abstract contract is not being used in the current UniV3LiquidityAMO contract:

  abstract contract OracleLike {
    function read() external view virtual returns (uint);
  
    function uniswapPool() external view virtual returns (address);
  }

Recommendation

Recommend removing unnecessary code component.

Unnecessary Minter Validation

Code Position

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L120

Description

In the contract RdpxDecayingBonds, the mint function is intended to be invoked by the minter, however, the minter has been checked twice through onlyRole and require statement:

  function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) {
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
    uint256 bondId = _mintToken(to);
    bonds[bondId] = Bond(to, expiry, rdpxAmount);

    emit BondMinted(to, bondId, expiry, rdpxAmount);
  }

Recommendation

Recommend removing unnecessary validation for the gas/runtime optimiaztions.

Unnecessary _whenNotPaused in mint

Code Position

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L119

Description

The mint function includes the _whenNotPaused function to halt its operation when the contract is paused. However, this is redundant, as the _beforeTokenTransfer method already provides this functionality. The following call stack helps clarify the overlap:

mint
-> _mintToken
  -> _mint
    -> _beforeTokenTransfer

Recommendation

Recommend removing redundant checks to optimize the codebase.