Dopex - MohammedRizwan's results

Lines of code

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

Vulnerability details

Impact

The PerpetualAtlanticVaultLP.sol contract is based on the ERC4626 where the shares are calculated based on the deposit value. Contest readme.md specified this contracts as Contract for the Perpetual Atlantic Vault LP (ERC4626).

By depositing large amount as initial deposit, initial depositor can influence the future depositors value, deposit() is given as below,

File: contracts/perp-vault/PerpetualAtlanticVaultLP.sol

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

As seen in function, Shares are minted based on the deposit value. By depositing large amount as initial deposit, first depositor can take advantage over other depositors. Future depositors are forced for huge value of asset to deposit. It is not practically possible for all the users and this could directly affect on the attrition of users towards this system.

As seen in deposit(), it has

File: contracts/perp-vault/PerpetualAtlanticVaultLP.sol

  function previewDeposit(uint256 assets) public view returns (uint256) {
    return convertToShares(assets);
  }

  /// @notice Returns the amount of shares recieved for a given amount of assets
  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

    uint256 totalVaultCollateral = totalCollateral() +
      ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
  }

The function convertToShares() is responsible to return the corresponding share of a user with respect to his deposited assets.

One of the main problems with these tokens lies in the fact that the first depositor can manipulate the unit share price in a bad way. The ratio of totalSupply/totalAssets determines the corresponding share of a deposited asset. As it is clear from the ratio if the first user deposits 1 Wei, will get 1 Wei worth of share.

Now For example: consider a below scenario:

Consider, Given a vault with DAI as the underlying asset:

1. Alice (attacker) deposits initial liquidity of 1 wei DAI via deposit()
2. Alice receives 1e18 (1 wei) vault shares
3. Alice transfers 1 ether of DAI via transfer() to the vault to artificially inflate the asset balance without minting new shares. The asset balance is now 1 ether + 1 wei DAI -> vault share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18)
4. Bob (victim) deposits 100 ether DAI
5. Bob receives 0 shares
6. Bob receives 0 shares due to a precision issue. His deposited funds are lost.

The shares are calculated as following per the contract which can be checked here

    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
  }

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter to prevent such manipulations.

Note: It is to be noted here that openzeppelin has fixed ERC4626 inflation attack issue in their latest version. Reference here. The ERC4626 seems to be referred from solmate ERC4626 implementation and it looks very much same which can be checked here, However, consider using openzeppelin ERC4626 for vault in contract and can be checked here

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

In UniV2LiquidityAmo.sol, _sendTokensToRdpxV2Core() function is used to send back the tokens to rdpxV2Core contract address.

File: contracts/amo/UniV2LiquidityAmo.sol

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

The issue here the function does not Syncs asset reserves with contract balances that is sent back to rdpxV2Core contract address.

Here, tokenA and tokenB are the rdpx and weth addresses respectively.

  struct Addresses {
    // token A address
    address tokenA; // rdpx
    // token B address
    address tokenB; // weth

As they are sent back to rdpxV2Core contract address, they needs to sync the token balances as these are the part of reserveAsset array

  /* Inital tokens in the reserve
     index0: ZERO address
     index1: weth
     index2: rdpx
     index3: dpxEth
     index4: crv
  */

  /// @notice Array containg the reserve assets
  ReserveAsset[] public reserveAsset;

This is actually synced by this function in RdpxV2Core.sol

It is to be noted that, This is only the case with UniV2LiquidityAmo.sol, However if you see UniV3LiquidityAmo.sol _sendTokensToRdpxV2Core() function has sync the token balances which can be seen as below,

File: contracts/amo/UniV3LiquidityAmo.sol

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

To avoid the unexpected accounting behavior, token balances should be sync.

Proof of Concept

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

Tools Used

Manual review

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

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

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

Assessed type

Other

Summary

Low Risk Issues

[L‑01] openzeppelin's setupRole() is deprecated

Contracts has used _setupRole() in functions for setting the roles. However _setupRole() is deprecated by openzeppelin in favor of _grantRole(). This can be checked and confirmed at openzeppelin releases here

There are below instances of this issue: Instance 1 in UniV2LiquidityAmo.sol at L-58

Instance 2 in UniV3LiquidityAmo.sol at L-80

Instance 3 in RdpxV2Bond.sol at L-25,L-26

Instance 4 in RdpxV2Core.sol at L-125

Instance 5 in RdpxDecayingBonds.sol at L-61,L-62

Instance 6 in PerpetualAtlanticVault.sol at L-126

Instance 7 in ReLPContract.sol at L-80

Recommended Mitigation steps

Use _grantRole() instead of _setupRole()

[L‑02] set max limit of slippageTolerance in setSlippageTolerance()

setSlippageTolerance() is used to set the slippageTolerance which is used in swaps 1e8 precision. The contract has bydefault a slippageTolerance = 5e5 which is equivalent to 0.5% and this function is used to set same in future. The issue here is it checks the min slippageTolerance must be greater than zero but it does not check the maximum slippageTolerance which can be set to any value by admin.

There is instance of this issue:

Another instance of this issue:

Recommended Mitigation steps

Add check of max slippageTolerance so that it can be set within limits by admin. This must be ensured for users safety in case of malicious admin or admin may set the slippageTolerance to any value.

[L‑03] Calls inside loops that may address DoS

In UniV2LiquidityAmo.sol, emergencyWithdraw() function, Calls to external contracts inside a loop are dangerous because it could lead to DoS if one of the calls reverts or execution runs out of gas. Such issue also introduces chance of problems with the gas limits.

Per SWC-113: External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract. To minimize the damage caused by such failures, it is better to isolate each external call into its own transaction that can be initiated by the recipient of the call.

Reference link- https://swcregistry.io/docs/SWC-113

Below are few instance of this issue: Instance 1 in UniV2LiquidityAmo.sol at L-149

File: contracts/amo/UniV2LiquidityAmo.sol

  function emergencyWithdraw(
    address[] calldata tokens
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    IERC20WithBurn token;

    for (uint256 i = 0; i < tokens.length; i++) {
      token = IERC20WithBurn(tokens[i]);
>>    token.safeTransfer(msg.sender, token.balanceOf(address(this)));
    }

    emit LogEmergencyWithdraw(msg.sender, tokens);

Recommended Mitigation Steps

1. Avoid combining multiple calls in a single transaction, especially when calls are executed as part of a loop
2. Always assume that external calls can fail
3. Implement the contract logic to handle failed calls

[L‑04] Admin may not receive usdc/usdt if blacklisted in case of emergency

In UniV2LiquidityAmo.sol, emergencyWithdraw() is used to transfer the tokens in contract to msg.sender i.e admin address. However it is to be noted here that tokens can also be usdc or usdt whihc has user black listing feature. If the admin address is blacklisted then as the usdc or usdt can not be transferred to admin address. Therefore in case of emergency for which the function has been designed for wont work for usdc or usdt tokens.

There is 1 instance of this issue:

File: contracts/amo/UniV2LiquidityAmo.sol

  function emergencyWithdraw(
    address[] calldata tokens
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    IERC20WithBurn token;

    for (uint256 i = 0; i < tokens.length; i++) {
      token = IERC20WithBurn(tokens[i]);
      token.safeTransfer(msg.sender, token.balanceOf(address(this)));
    }

    emit LogEmergencyWithdraw(msg.sender, tokens);
  }

Recommended Mitigation steps

Add function to transfer the usdc,usdt or function which can blacklist the users from receiving tokens and put the admin access control to it.

For example:

  function emergencyWithdraw(address token, address recipient) external onlyRole(DEFAULT_ADMIN_ROLE) {
    IERC20WithBurn token;
      token.safeTransfer(recipient, token.balanceOf(address(this)));
    }

    emit LogEmergencyWithdraw(recipient, token);
  }

[L‑05] Insufficient validations in setAddresses() can cause both tokenA and tokenB same

In UniV2LiquidityAmo.sol, setAddresses() is used to set different addresses which is shown below,

File: contracts/amo/UniV2LiquidityAmo.sol

  function setAddresses(
    address _tokenA,
    address _tokenB,
    address _pair,
    address _rdpxV2Core,
    address _rdpxOracle,
    address _ammFactory,
    address _ammRouter
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(
      _tokenA != address(0) &&
        _tokenB != address(0) &&
        _pair != address(0) &&
        _rdpxV2Core != address(0) &&
        _rdpxOracle != address(0) &&
        _ammFactory != address(0) &&
        _ammRouter != address(0),
      "reLPContract: address cannot be 0"
    );
    addresses = Addresses({
      tokenA: _tokenA,
      tokenB: _tokenB,
      pair: _pair,
      rdpxV2Core: _rdpxV2Core,
      rdpxOracle: _rdpxOracle,
      ammFactory: _ammFactory,
      ammRouter: _ammRouter
    });
  }

The issue here is, it is possible the tokenA and tokenB can be same however this should not be the desired behavior as tokenA will be set to rdpx and tokenB to weth but there is no validation that checks both are not same.

There instance of this issue:

Another instance of this issue:

Recommended Mitigation steps

File: contracts/amo/UniV2LiquidityAmo.sol

  function setAddresses(
    address _tokenA,
    address _tokenB,
    address _pair,
    address _rdpxV2Core,
    address _rdpxOracle,
    address _ammFactory,
    address _ammRouter
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(
      _tokenA != address(0) &&
        _tokenB != address(0) &&
        _pair != address(0) &&
        _rdpxV2Core != address(0) &&
        _rdpxOracle != address(0) &&
        _ammFactory != address(0) &&
        _ammRouter != address(0),
      "reLPContract: address cannot be 0"
    );
+   require(_tokenA != _tokenB, "tokens can not be same");
    addresses = Addresses({
      tokenA: _tokenA,
      tokenB: _tokenB,
      pair: _pair,
      rdpxV2Core: _rdpxV2Core,
      rdpxOracle: _rdpxOracle,
      ammFactory: _ammFactory,
      ammRouter: _ammRouter
    });
  }

[L‑06] emergencyWithdraw() functionality should be added in UniV3LiquidityAmo.sol like other contracts

emergencyWithdraw() function is used in case of emergency, However this is missing in UniV3LiquidityAmo.sol contract. Other contracts like RdpxDecayingBonds.sol, PerpetualAtlanticVault.sol, RdpxV2Core.solandUniV2LiquidityAmo.solhaveemergencyWithdraw()`. This should be added in contract to use incase of emergency.

There is 1 instance of this issue:

Recommedended Mitigation steps

Add emergencyWithdraw() function in contract. Refer the above listed contracts for similar implementation.

[L‑07] TickMath.sol might revert in solidity version 0.8.19 as used in contracts

UniswapV3’s TickMath library was changed to allow compilations for solidity version 0.8. However, adjustments to account for the implicit overflow behavior that the contract relies upon were not performed.

This has been extensively used in most of contracts. This also includes contracts which are in scope as well as out of scope contracts.

Lets's take an example: UniV3LiquidityAmo.sol is compiled with version 0.8.19 and has used this library in contract which can be seen as below,

import "../uniswap_V3/libraries/TickMath.sol";

In the worst case, it could be that the library always reverts (instead of overflowing as in previous versions), leading to a broken contracts(contract which have used it).

Upon finding the version of TickMath.sol, it has used pragma solidity >=0.5.0 which can be seen as below,

pragma solidity >=0.5.0;

The same pragma solidity >=0.5.0; instead of pragma solidity >=0.5.0 <0.8.0; adjustments needs to made for the implicit overflow behavior in used contracts.

Recommended Mitigation steps

Follow the implementation of the official TickMath 0.8 branch which uses unchecked blocks for every function.

[L‑08] Update solc version and use unchecked in Uniswap related libraries like LiquidityAmounts.sol

The LiquidityAmounts.sol has been used in most of contracts including in UniV3LiquidityAmo.sol and it is referenced from Uniswap codebase which is intended to work with Solidity compiler <0.8. These older versions have unchecked arithmetic by default and the code takes it into account.

However, dopex contracs code is intended to work with Solidity compiler 0.8.19 which doesn't have unchecked arithmetic by default.

For example, UniV3LiquidityAmo.sol can be checked here

Hence, to port the code, it has to be turned on via unchecked keyword.

For example, type(uint).max reverts for v0.8 and returns type(uint).max for older version

Recommended Mitigation steps

1. Update the pragma of all the three files to 0.8.19 as it is used in all dopex contracts
2. Wrap all the function bodies of uniswap libraries like LiquidityAmounts.sol in unchecked.

[L‑09] Solmate safeTransferLib.sol functions does not check the codesize of the token address, which may lead to fund loss

In PerpetualAtlanticVaultLP.sol contract, The contract has used Solmate safeTransferLib.sol which doesn't check the existence of code at the token address. This is a known issue while using solmate's libraries.

Per the Solmate safeTransferLib.sol,

Note that none of the functions in this library check that a token has code at all! .....

Hence using safeTransferLib.sol library may lead to miscalculation of funds and may lead to loss of funds , because if safetransfer() and safetransferfrom() are called on a token address that doesn't have contract in it, it will always return success, bypassing the return value check. Due to this protocol will think that funds has been transferred and successful , and records will be accordingly calculated, but in reality funds were never transferred.

Proof of Concept

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

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

Recommended Mitigation Steps

Use openzeppelin's safeERC20 which takes care of token code existence.

[L‑10] An issue with TransferHelper.sol which is extensively used in UniV3LiquidityAmo.sol

UniV3LiquidityAmo.sol has used the TransferHelper.sol which is basically copied from uniswap. This TransferHelper library has been used for safeApprove(), safeTransfer() in UniV3LiquidityAmo.sol contract. The issue is the library functions does not check the token address existence and it also does not check the address(0) validations for token address. There was an exploit happened due to not checking the address(0) because of the use of this library which can be referenced here,

According to Certik’s analysis:

One of the root causes of the vulnerability was the fact that tokenAddress.safeTransferFrom() does not revert when the tokenAddress is the zero (null) address (0x0…000)

Recommended Mitigation steps

Check token address is not address(0) and check the existence of token address in UniV3LiquidityAmo.sol OR it should be fixed in TransferHelper.sol which will reflect the changes wherever it is being used.

[L‑11] Do not hard code the contract address, pass as constructor argument in UniV3LiquidityAmo.sol

In UniV3LiquidityAmo.sol, univ3_factory, univ3_positions, univ3_router has been hardcoded. This should not be hardcoded in constructor though the contracts will only be deployed on Arbitrum for now but in case it deploys on other chains, such implementation is not recommended. These addresses should be passed as an constructor arguments.

There are 3 instances of this issue:

  constructor(address _rdpx, address _rdpxV2Core) {
    rdpx = _rdpx;
    rdpxV2Core = _rdpxV2Core;

    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

    univ3_factory = IUniswapV3Factory(
      0x1F98431c8aD98523631AE4a59f267346ea31F984
    );
    univ3_positions = INonfungiblePositionManager(
      0xC36442b4a4522E871399CD717aBDD847Ab11FE88
    );
    univ3_router = ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);
  }

Recommended Mitigation steps

+ //  univ3_factory = 0x1F98431c8aD98523631AE4a59f267346ea31F984
+ //  univ3_positions = 0xC36442b4a4522E871399CD717aBDD847Ab11FE88
+ //  univ3_router = 0xE592427A0AEce92De3Edee1F18E0157C05861564


-  constructor(address _rdpx, address _rdpxV2Core) {
+  constructor(address _rdpx, address _rdpxV2Core, _univ3_factory, _univ3_positions, _univ3_router) {
    rdpx = _rdpx;
    rdpxV2Core = _rdpxV2Core;

    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);

-    univ3_factory = IUniswapV3Factory(
-      0x1F98431c8aD98523631AE4a59f267346ea31F984
-    );
+    univ3_factory = IUniswapV3Factory(_univ3_factory);
-    univ3_positions = INonfungiblePositionManager(
-      0xC36442b4a4522E871399CD717aBDD847Ab11FE88
-    );
+    univ3_positions = INonfungiblePositionManager(_univ3_positions);
-    univ3_router = ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);
+    univ3_router = ISwapRouter(_univ3_router);
  }

[L‑12] In RdpxDecayingBonds.sol, mint() does not verify that the bond expiry has passed/expired

In RdpxDecayingBonds.sol contract, expiry is used in mint() function for the expiry timestamp of the bond. The issue here mint() is not validating if the expiry is still running or not, Without expiry verification the functions keeps allowing to be made after the stipulated time or expiry or it may end immediately without letting any one know.

File: contracts/decaying-bonds/RdpxDecayingBonds.sol

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

As it can be seen, expiry is not validated to be greater than block.timestamp. Therefore considering the impact the expiry must be validated to greater than block.timestamp.

Recommended Mitigation steps

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

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

[L‑13] In DpxEthToken.sol, mint(), burn() and burnFrom() should only be called when the contract is not paused

In DpxEthToken.sol, mint(), burn() and burnFrom() can be called anytime though the contract has pause and unpause functionality.

File: contracts/dpxETH/DpxEthToken.sol

  function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
    _mint(to, amount);
  }

  function burn(
    uint256 _amount
  ) public override(ERC20Burnable, IDpxEthToken) onlyRole(BURNER_ROLE) {
    _burn(_msgSender(), _amount);
  }

  function burnFrom(
    address account,
    uint256 amount
  ) public override onlyRole(BURNER_ROLE) {
    _spendAllowance(account, _msgSender(), amount);
    _burn(account, amount);
  }

There are 3 instances of this issue

It is to be noted that _beforeTokenTransfer() can be called when the contract is not paused. I argue to call the mint and burn functions when the contract is not paused.

In other contracts like RdpxDecayingBonds.sol mint() can only be called when the contract is not paused. Similarly this can considered here too.

Recommended Mitigation steps

  function mint(address to, uint256 amount) public onlyRole(MINTER_ROLE) {
+   _whenNotPaused();
    _mint(to, amount);
  }

  function burn(
    uint256 _amount
  ) public override(ERC20Burnable, IDpxEthToken) onlyRole(BURNER_ROLE) {
+   _whenNotPaused();
    _burn(_msgSender(), _amount);
  }

  function burnFrom(
    address account,
    uint256 amount
  ) public override onlyRole(BURNER_ROLE) {
+   _whenNotPaused();
    _spendAllowance(account, _msgSender(), amount);
    _burn(account, amount);
  }

[L‑14] addToContractWhitelist() does not check the contract code existence before whitelisting

In PerpetualAtlanticVault.sol, addToContractWhitelist() function is used to whitelist contract address. However it does not check the contract code existence before whitlisting it. It is possible that any EOA address or any malicious or phoney address may get whitelisted by admin.

There is 1 instance of this issue:

File: contracts/perp-vault/PerpetualAtlanticVault.sol

  function addToContractWhitelist(
    address _contract
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _addToContractWhitelist(_contract);
  }

Recommended Mitigation steps

Check code existence check before whitelisting.

  function addToContractWhitelist(
    address _contract
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
+   require(_contract.code.lenght != 0, "EOA address can not be whitelisted");
    _addToContractWhitelist(_contract);
  }

[L‑15] In RdpxV2Bond.sol, mint() should be called when the contract is not paused

In RdpxV2Bond.sol, mint() can be called anytime though the contract has pause and unpause functionality.

File: contracts/core/RdpxV2Bond.sol

  function mint(
    address to
  ) public onlyRole(MINTER_ROLE) returns (uint256 tokenId) {
    tokenId = _tokenIdCounter.current();
    _tokenIdCounter.increment();
    _mint(to, tokenId);
  }

There are 1 instances of this issue

It is to be noted that _beforeTokenTransfer() can be called when the contract is not paused. I argue to call the mint() when the contract is not paused.

In other contracts like RdpxDecayingBonds.sol mint() can only be called when the contract is not paused. Similarly this can considered here too.

Recommended Mitigation steps

  function mint(
    address to
  ) public onlyRole(MINTER_ROLE) returns (uint256 tokenId) {
+   _whenNotPaused();
    tokenId = _tokenIdCounter.current();
    _tokenIdCounter.increment();
    _mint(to, tokenId);
  }

[L‑16] Misleading Natspec on getLpPrice() in UniV2LiquidityAmo.sol

getLpPrice() Natspec states Price is in 1e8 Precision, however this is not correct, The price returned in 1e18.

There is 1 instances of this issue:

File: contracts/amo/UniV2LiquidityAmo.sol

  /**
   * @notice Returns the price of a rDPX/ETH Lp token against the alpha token
   * @dev    Price is in 1e8 Precision
   * @return uint256 LP price
   **/
  function getLpPrice() public view returns (uint256) {
    return IRdpxEthOracle(addresses.rdpxOracle).getLpPriceInEth();
    }

I have confirmed the issue by recheching the RdpxEthOracle.sol which is given as below,

    /// @dev Returns the price of LP in ETH in 1e18 decimals
    function getLpPriceInEth() external view override returns (uint) {

Recommended Mitigation steps

  /**
   * @notice Returns the price of a rDPX/ETH Lp token against the alpha token
-   * @dev    Price is in 1e8 Precision
+   * @dev    Price is in 1e18 Precision

   * @return uint256 LP price
   **/
  function getLpPrice() public view returns (uint256) {
    return IRdpxEthOracle(addresses.rdpxOracle).getLpPriceInEth();
    }

[L‑17] deadline should be passed as an argument instead of hardcoding it

In contracts like UniV2LiquidityAmo.sol, UniV3LiquidityAmo.sol and other contracts functions like addLiquidity(), removeLiquidity() and other functions have used deadline. The deadline is hardcoded in contract. block.timestamp can be manipulated by miners/stakers. Therefore hardcoding deadline is not recommended.

Recommended Mitigation steps

Pass the deadline as an argument for all functions using it in contracts.