Dopex - catellatech's results

Lines of code

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

Vulnerability details

Summary

The first depositor can mint a very small number of shares, then donate assets to the Vault. Thereby he manipulates the exchange rate and later depositors lose funds due to rounding down in the number of shares they receive.

PoC

ERC4626 vaults are subject to a share price manipulation attack that allows an attacker to steal underlying tokens from other depositors:

1. Alice is the first depositor of the vault
2. Alice deposits 1 wei of token A
3. in the deposit function, the amount of shares is calculated using the deposit function:
4. since Alice is the first depositor (totalSupply is 0), she gets 1 share (1 wei)
5. Alice then sends 9999999999999999999 token A (10e18 - 1) to the vault
6. the price of 1 share is 10 token A now: Alice is the only depositor in the vault, she's holding 1 wei of shares, and the balance of the pool is 10 token A;
7. Bob deposits 19 token A and gets only 1 share due to the rounding in the convertToShares function: 19e18 * 1 / 10e18 == 1
8. Alice redeems her share and gets a half of the deposited assets, 14.5 tokens
9. Bob redeems his share and gets only 14.5 amount of token, instead of the 19 tokens he deposited

Impact

Tokens can be stolen from depositors in PerpetualAtlanticVaultLP contract by manipulating the price of a share.

Tools Used

Manual review

Recommended Mitigation Steps

This issue can be mitigated by requiring a minimum deposit of assets. Thereby the attacker cannot manipulate the exchange rate to be so low as to enable this attack.

Assessed type

Other

QA report for Dopex contest by Catellatech

[01] In the contract RdpxDecayingBonds.mint the require is redundant

In the mint function, there is essentially a double requirement of the same thing. Consider removing it, as the modifier does what you wrote in the require statement.

PoC

We removed the require statement and run the tests and work as we expected.

    <!-- 1. We commented the require and run the test -->
    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);
    }

Expected output:

[PASS] testMint() (gas: 208805)
Logs:
  the test is complete without the require in the function bacause its has the modifier

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

[02] The RdpxV2Core.approveContractToSpend() function doesn't perform as described in its NatSpec and function name

In the RdpxV2Core.approveContractToSpend() function, the following is described:

@notice Approve a contract to spend a certain amount of tokens

This is not accurate, as the function does not perform a check to verify whether the _spender address is indeed a contract, with the assistance of libraries like OpenZeppelin offers.

this is the function

  /**
   * @notice Approve a contract to spend a certain amount of tokens
   * @dev    Can only be called by admin
   * @param  _token the address of the token to approve
   * @param  _spender the address of the contract to approve
   * @param  _amount the amount to approve
   */
  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_token != address(0), 17);
    _validate(_spender != address(0), 17); // validate that the spender is a contract address
    _validate(_amount > 0, 17);
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Proof of Concept

     function testEOACanBeApproved() public {
        // Lets make the rdpxV2Core contract has balance
        weth.transfer(address(rdpxV2Core), 25 * 1e18);

        address alice = makeAddr("alice");
        
        console.log("Balance of rdpxV2Core before being approved to an EOA:", weth.balanceOf(address(rdpxV2Core)));
        rdpxV2Core.approveContractToSpend(address(weth), alice, 25 * 1e18);

        // now the address that the admin approved can steal the tokens approved by admin
        console.log("EOA make the tx");
        vm.startPrank(alice);
        weth.transferFrom(address(rdpxV2Core), alice, 25 * 1e18);
        console.log("rdpxV2Core balance after rugpull from the alice address :", weth.balanceOf(address(rdpxV2Core)));
        vm.stopPrank();
    }

the output is:

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testEOACanBeApproved() (gas: 85071)
Logs:
  Balance of rdpxV2Core before being approved to an EOA: 25000000000000000000
  EOA make the tx
  rdpxV2Core balance after rugpull from the alice address : 0

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

Recommended Mitigation Steps

Add another check to ensure that the _spender address is indeed a contract and aligns with the function name approveContractToSpend, as well as with the documentation provided by the team: Approve a contract to spend a certain amount of tokens.

[03] In the RdpxDecayingBonds:emergencyWithdraw function exist a wrong assumption

The contract does not allow receiving native currency, but the RdpxDecayingBonds:emergencyWithdraw function has a mechanism to be able to transfer native currency. Speaking with the sponsor, he let me know the following:

psytama — hoy a las 9:55
ya the function used is just a basic function we add so its not optimized but the contract is not supposed to handle funds so its only there incase someone sends it tokens by mistake.

But this is wrong because the contract does not allow receiving native currency through a receive/fallback function.

the function looks like

/**
     * @notice Transfers all funds to msg.sender
     * @dev    Can only be called by the owner
     * @param  tokens The list of erc20 tokens to withdraw
     * @param  transferNative Whether should transfer the native currency
     * @param  to The address to transfer the funds to
     * @param  amount The amount to transfer
     * @param  gas The gas to use for the transfer
     *
     */
    function emergencyWithdraw(
        address[] calldata tokens,
        bool transferNative,
        address payable to,
        uint256 amount,
        uint256 gas
    ) external onlyRole(DEFAULT_ADMIN_ROLE) {
        _whenPaused();
        if (transferNative) {
            (bool success,) = to.call{value: amount, gas: gas}("");
            require(success, "RdpxReserve: transfer failed");
        }

        IERC20WithBurn token;

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

PoC to proof that the contract cannot receive "accidentaly"

    function testCannotReceiveNativeCurrency() public {
        // The contract cannot receive native currency "accidentaly"
        (bool revertsAsExpected, ) = address(rdpxDecayingBonds).call{value: 1 ether}("");
        assertFalse(revertsAsExpected);
    } 

the output is:

Running 1 test for tests/RdpxDecayingBondsTest.t.sol:RdpxDecayingBondsTest
[PASS] testLowLevelCallRevert() (gas: 11852)
Traces:
  [11852] RdpxDecayingBondsTest::testLowLevelCallRevert()
    ├─ [45] RdpxDecayingBonds::fallback{value: 1000000000000000000}()
    │   └─ ← "EvmError: Revert"
    └─ ← ()

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

[04] Remove/resolve the comments like the following

Reviewing the contract tests, I was able to notice a comment in the unit tests for the vault contracts, specifically in the PerpetualAtlanticVault.settle() function's test. The comment in the test looks like this:

/tests/perp-vault/Unit.t.sol

129: // bug: updateUnderlyingPrice does not update price, getUnderlyingPrice() will use amm price
130:  function testSettle() public 

According to the comments in the function, this is what the function does:

   @notice   The function asset settles the options by transferring rdpx 
              from the rdpxV2Core and transfers eth to the rdpx rdpxV2Core 
              contract at the option strike price. Will also the burn the 
              option tokens from the user.

It is recommended to fix or remove this comment, in case the team knows whether the updateUnderlyingPrice function works or not.

[05] Potential accidental inclusion of ERC20Burnable in xETHDpxEthToken contract

• https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/dpxETH/DpxEthToken.sol#L14

The xETHDpxEthToken contract inherits from ERC20Burnable, which defines public methods to allow burning of tokens.

Presumably, this was added accidentally as a mistake in order to implement the burn() function in xETHDpxEthToken. The internal function _burn is already provided in the base implementation of ERC20 (OpenZepplin library), and doesn’t need any extra implementation, which may increase the attack surface of the protocol.

The recommendation is to remove ERC20Burnable from the inheritance list of xETHDpxEthToken.

[06] Multiple uint mappings can be combined into a single mapping of a uint to a struct, where appropriate

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

Contract that we recomment to apply the change

contracts/perp-vault/PerpetualAtlanticVault.sol

66: mapping(uint256 => uint256) public fundingPaymentsAccountedFor;
73: mapping(uint256 => uint256) public totalFundingForEpoch;
76: mapping(uint256 => uint256) public optionsPerStrike;
79: mapping(uint256 => uint256) public latestFundingPerStrike;
82: mapping(uint256 => uint256) public fundingRates;

Recommended Mitigation Steps

We recommend applying your own best practices, as you did in the RdpxDecayingBonds contract.

[07] In the PerpetualAtlanticVaultLP.constructor missing check of input validation

In the constructor, it is not checked that the address of _rdpxRdpxV2Core is different from address(0). It is recommended to follow your applied best practices and properly validate this important input.

The same recommendation can be applied on the RdpxV2Core.constructor with the weth address.

[08] Inconsistent Use of NatSpec

NatSpec is a boon to all Solidity developers. Not only does it provide a structure for developers to document their code within the code itself, it encourages the practice of documenting code. When future developers read code documented with NatSpec, they are able to increase their capacity to understand, upgrade, and fix code. Without code documented with NatSpec, that capacity is hindered.

The DOPEX codebase does have a high level of documentation with NatSpec. However there are numerous instances of functions missing NatSpec.

Possible Risks

1. Lack of understanding of code without adequate documentation.
2. Difficulty in understanding, upgrading, and fixing code without documentation.

Recommended Mitigation Steps

• Practice consistent use of NatSpec on all parts of the project codebase.
• Include the need for NatSpec comments for code reviews to pass.

[09] Inconsistent coding style

An inconsistent coding style can be found throughout the codebase. Some examples include: Some of the parameter names in certain functions lack the underscores as seen in other cases. To enhance code readability and adhere to the Solidity style guide standards, it is recommended to incorporate underscores in the parameter names. This practice contributes to maintaining a consistent and organized codebase.

In some functions, underscores are used:

Contract: UniV2LiquidityAmo
 function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) 

while in others they are not present:

Contract: UniV2LiquidityAmo
  function addLiquidity(
    uint256 tokenAAmount,
    uint256 tokenBAmount,
    uint256 tokenAAmountMin,
    uint256 tokenBAmountMin
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 tokenAUsed, uint256 tokenBUsed, uint256 lpReceived)

To improve the overall consistency and readability of the codebase, consider adhering to a more consistent coding style by following a specific style guide.

[10] Empty blocks should be removed or emit something

In Solidity, any function defined in a non-abstract contract must have a complete implementation that specifies what to do when that function is called.

contracts/perp-vault/PerpetualAtlanticVaultLP.sol

231:function _beforeTokenTransfer(
        address from,
        address,
        uint256
    ) internal virtual {}

Recommended Mitigation Steps

Emmit something in the function.

[11] Old version of OpenZeppelin Contracts used

Using old versions of 3rd-party libraries in the build process can introduce vulnerabilities to the protocol code.

• Latest is 4.9.3 (as of July 28, 2023), version used in project is 4.9.0

Risks

• Older versions of OpenZeppelin contracts might not have fixes for known security vulnerabilities.
• Older versions might contain features or functionality that have been deprecated in recent versions.
• Newer versions often come with new features, optimizations, and improvements that are not available in older versions.

Recommendation

Review the latest version of the OpenZeppelin contracts and upgrade

[12] In some important functions trough the scope should have better manage of events

In some functions where the admin can set or update certain sensitive information, only the new information is tracked in the event, omitting the previous information. It should be considered to add the input of the previous information in the event to maintain transparency.

Function that we recomment to add the old information in the event

contracts/core/RdpxV2Core.sol

180: function setRdpxBurnPercentage
193: function setRdpxFeePercentage
228: function setBondMaturity
441: function setBondDiscount

[13] In the UniV2LiquidityAmo.approveContractToSpend() function should emit some event

Events provide a way for auditing and traceability, as they publicly record the actions and changes made in a smart contract. This can be useful to verify the authenticity of certain transactions or activities. And in this case the function is very important:

Function that we recomment to have event

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(_token != address(0), "reLPContract: token cannot be 0");
    require(_spender != address(0), "reLPContract: spender cannot be 0");
    require(_amount > 0, "reLPContract: amount must be greater than 0");
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Other functions that should have events:

contracts/reLP/ReLPContract.sol

171: function setLiquiditySlippageTolerance
186: function setSlippageTolerance

Recommended Mitigation Steps

Add some event in the function to keep the transparency with the users.

[14] Crucial information is missing on important functions in all contracts

In the constructor functions it is not specified in the documentation if the admin will be an EOA or a contract, for example in the UniV2LiquidityAmo and UniV3LiquidityAmo contract. Consider improving the docstrings to reflect the exact intended behaviour, and using Address.isContract function from OpenZeppelin’s library to detect if an address is effectively a contract.

[15] In some contracts, we find inconsistencies regarding uint256

Some developers prefer to use uint256 because it is consistent with other uint data types, which also specify their size, and also because making the size of the data explicit reminds the developer and the reader how much data they've got to play with, which may help prevent or detect bugs. insecure and more error-prone.

Recommended Mitigation Steps

Use forge fmt to format the contracts.

[16] We suggest to use named parameters for mapping type

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18. For example:

mapping(address account => uint256 balance); 

[17] We suggest using the OpenZeppelin SafeCast library

We have noticed that in the main contract RdpxV2Core.__curveSwap() implements some type conversions. Explicited first converted uint256 to int256 and then int256 to int128, use the safeCast and implement the following functions:

----------------------- uint256 to int256 ----------------------------------

    /**
     * @dev Converts an unsigned uint256 into a signed int256.
     *
     * Requirements:
     *
     * - input must be less than or equal to maxInt256.
     */
    function toInt256(uint256 value) internal pure returns (int256) {
        // Note: Unsafe cast below is okay because `type(int256).max` is guaranteed to be positive
        if (value > uint256(type(int256).max)) {
            revert SafeCastOverflowedUintToInt(value);
        }
        return int256(value);
    }

----------------------- int256 to int128 ----------------------------------
    /**
     * @dev Returns the downcasted int128 from int256, reverting on
     * overflow (when the input is less than smallest int128 or
     * greater than largest int128).
     *
     * Counterpart to Solidity's `int128` operator.
     *
     * Requirements:
     *
     * - input must fit into 128 bits
     */
    function toInt128(int256 value) internal pure returns (int128 downcasted) {
        downcasted = int128(value);
        if (downcasted != value) {
            revert SafeCastOverflowedIntDowncast(128, value);
        }
    }

We recommend using the OpenZeppelin SafeCast library to make the project more robust and take advantage of the gas optimizations and best practices provided by OpenZeppelin.

[18] Use a single file for all system-wide constants

In some contracts, there are many constants used in the system. It is recommended to store all constants in a single file (for example, Constants.sol) and use inheritance to access these values.

This helps improve readability and facilitates future maintenance. It also helps with any issues, as some of these hard-coded contracts are administrative contracts.

Constants.sol should be used and imported in contracts that require access to these values. This is just a suggestion.

[19] Use of deprecated function

The team is working with OZ's Access Control, the _setupRole function is deprecated. OZ documentation

    * NOTE: This function is deprecated in favor of {_grantRole}.
     */
    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

Dopex Protocol Smart Contracts Analysis

Description overview of Dopex Protocol

The Dopex protocol has introduced a new version called rDPX V2, which brings an interesting way to reward those who write options on their platform using a rebate system. Additionally, they've created a new synthetic coin called dpxETH, which is tied to the price of ETH. The idea behind this coin is to use it for earning higher yields with ETH and as a collateral asset for future options products on Dopex.

The bonding process in rDPX V2 is how new dpxETH coins can be created. When someone bonds, they receive a "receipt" that represents a combination of ETH and dpxETH in a system called Curve. Through this bonding process, new dpxETH coins are minted, and to ensure they always have backing, there are reserves of rDPX and ETH called "Backing Reserves." These reserves are controlled by entities called "AMOs."

To achieve a safe and controlled scaling of rDPX V2 and dpxETH, the protocol has adopted an idea called "AMO ideology" from Frax Finance, which is another similar project. Essentially, they are drawing inspiration from how Frax Finance manages its system to ensure that the new version of Dopex can grow and operate in a stable and sustainable manner.

Summary

1- Approach we followed when reviewing the code

To begin, we assessed the code's scope, which guided our approach to reviewing and analyzing the project.

Scope

• 1. UniV2LiquidityAMO.sol:

  • This contract is utilized to manage liquidity in the Uniswap V2 token pair, as well as to execute automated exchange operations. The contract encompasses administrative functions, AMO (Automated Market Maker Operator) functions, and view functions to oversee a variety of liquidity-related operations and token exchanges.

• 2. UniV3LiquidityAmo.sol:

  • This contract is designed to interact with the Uniswap V3 protocol and perform liquidity management operations.

• 3. RdpxV2Core.sol:

  • This contract is the core of the protocol and is responsible for managing crucial operations such as interacting with delegates, asset management, option settlement, balance synchronization, and repeg handling. Internal _validate functions are used to check conditions throughout the contract and generate exceptions if a condition is not met. This contract ensures that operations are carried out in an authorized manner and in accordance with predefined protocol rules.

• 4. RdpxV2Bond.sol:

  • this contract defines an ERC721-based NFT token with the ability to mint new tokens, pause/unpause token transfers, and enforce checks during token transfers. It leverages functionality from various OpenZeppelin contracts to implement these features.
• 5. RdpxDecayingBonds.sol:
  • RdpxDecayingBonds contract provides a way to mint and manage decaying rDPX bonds as non-fungible tokens. Users with specific roles can create bonds, perform emergency withdrawals, and query the bonds they own. The contract is designed to be paused in critical situations and features a structure that ensures security and control over operations at all times.

• 6. DpxEthToken.sol:

  • This contract defines a synthetic token with ERC-20 functionalities and adds features for pausing, minting, and burning tokens. The contract leverages role-based access control to manage the interactions and operations on the token.

• 7. PerpetualAtlanticVault.sol:

  • This contract represent a vault for perpetual atlantic rDPX PUT options. It is designed to handle option issuance, settlement, and funding calculation. The contract utilizes various roles and OpenZeppelin libraries to ensure its functionality and security.

• 8. PerpetualAtlanticVaultLP.sol:

  • this contract handles liquidity provision for the PerpetualAtlanticVault contract, allowing users to deposit and withdraw assets, while also managing aspects related to the PerpetualAtlanticVault contract and calculating conversions and previews for operations. And use the standard ERC4626.
• 9. ReLPContract.sol:
  • this contract manages the process of re-liquidity provisioning for a DEX pair using the Uniswap V2 protocol. It calculates the optimal amount of Token A reLP to remove from the pool, performs swaps, and adds the new liquidity back to the pool. The contract is designed to ensure efficient and accurate re-liquidity provision while syncing balances with other relevant contracts.

Privileged Roles

There are several important roles in the system.

• DEFAULT_ADMIN_ROLE: This role is essential in every contract. It grants administrative permissions and control over critical functions. The holder of this role can manage other roles, configure contract parameters, and perform various administrative tasks.

 constructor() {
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
  }

• MANAGER_ROLE: This role is present in the PerpetualAtlanticVault contract. It is used for general contract management, including setting addresses, adjusting parameters, and controlling administrative functions. The manager role helps ensure proper operation and configuration.

 bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE");

• MINTER_ROLE - PAUSER_ROLE - BURNER_ROLE :Found in contracts like DpxEthToken, RdpxDecayingBonds and RdpxV2Bond this role permits the creation or minting of new tokens or bonds. It's important for controlling the issuance of new tokens, maintaining the token supply, and potentially providing incentives.

  bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
  bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
  bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

Initially, the accounts responsible for fully controlling these roles will be managed by the Dopex team as confirmed by the sponsor

Psytama — 31/08/2023 at 7:24. 
Yes, they will be controlled by the protocol through a multisig. 

Given the level of control that these roles possess within the system, users must place full trust in the fact that the entities overseeing these roles will always act correctly and in the best interest of the system and its users.

After grasping the code's functionality, we carefully analyzed and audited it step by step. Our goal was to ensure its security and proper operation.

2- Analysis of the code base

The main most important contracts of rDPX V2.

• Here's a detailed diagrams of the essential components of these contracts:

1. RdpxV2Core:

2. PerpetualAtlanticVault:

3. PerpetualAtlanticVaultLP:

4. UniV3LiquidityAmo:

5. RdpxV2Bond:

6. RdpxDecayingBonds:

7. ReLPContract:

What's unique? What's using existing patterns?

• Unique Features:

  • Decaying Bonds: Issuance of ERC721 decaying bonds offering rebates and indirect rDPX emission.

• Utilization of Existing Patterns:

  • Token Standards: ERC721 and ERC20 token standards for bonds, LP tokens, and assets.
  • Liquidity Pools and AMOs: Utilizing Uniswap V2 and V3 functions for liquidity and AMO operations.
  • Staking and Rewards: Implementing staking and proportional rewards mechanisms.
  • Oracle Integration: Using oracles for accurate price information.
  • Access Control: Role-based access control for secure interactions.
  • Strategy Contracts: Automating liquidity provision with strategy contracts.

3- Architecture of the rDPX V2

This diagram illustrates how the different contracts and modules interact within the rDPX V2 ecosystem.

• How a user interacts 🧑‍💻: Users interact with the rDPX V2 through their actions, such as:

  • Bonding and Interaction with Core Modules:

    • A user can initiate bonding processes through the Bonding Module. They can acquire rDPX Bond tokens, which represent their bond balance.
    • The user can interact with the Peg Stability module indirectly by participating in bonding processes. The module aims to maintain the peg of dpxETH.
    • Users can engage with the Backing Reserves module by providing reserves (such as rDPX or ETH) to be used by Automated Market Operators (AMOs)..

  • Liquidity Provision and Yield Generation:

    • Users can provide liquidity to the AtlanticPerpetualPoolLP, contributing to the liquidity of the AtlanticPerpetualPool.
    • By adding liquidity, users earn rewards which they can stake in the AtlanticPerpetualPoolStaking contract for further yield.

  • Option Trading:

    • Users can mint option tokens (NFTs) by interacting with the AtlanticPerpetualPool contract.
    • These option tokens can be traded or used for various purposes within the ecosystem.

  • Decaying Bonds and Loss Mitigation:

    • If a user experiences losses from Dopex Option Products, they might receive decaying bonds from the rDPX Decaying Bonds contract. These bonds act as a rebate for their losses.

  • Staking and Yield Farming:

    • Users can stake RdpxV2ReceiptToken in the RdpxV2ReceiptTokenStaking contract to earn boosted yields from the Curve LP and DPX incentives.

  • Utilizing Oracles:

    • Users can access real-time price information from the dpxETH/ETH Oracle and rDPX/ETH Oracle to make informed decisions.

  • AMO Interaction:

    • If users wish to use Uniswap V2 or V3 functions, they can do so through the respective AMO contracts. This involves actions like adding or removing liquidity and making swaps.

  • Redeeming Rewards and Tokens:

    • Users can redeem rewards earned from staking, providing liquidity, or participating in other activities within the ecosystem.

4- Documents

• The documentation of the Dopex rDPX V2 project is quite comprehensive and detailed, providing a solid overview of how rDPX V2 is structured and how its various aspects function. However, we have noticed that there is room for additional details, such as diagrams, to gain a deeper understanding of how different contracts interact and the functions they implement. With considerable enthusiasm, we have dedicated several days to creating diagrams for each of the contracts. We are confident that these diagrams will bring significant value to the protocol as they can be seamlessly integrated into the existing documentation, enriching it and providing a more comprehensive and detailed understanding for users, developers and auditors.

• We suggest including high-quality articles on Medium, as it's an excellent way to provide an in-depth understanding of many project topics, and it's a common practice in many blockchain projects.

5- Systemic & Centralization Risks

Here's an analysis of potential systemic and centralization risks in the provided contracts:

UniV2LiquidityAmo:

• Systemic Risks:

  • Calculation errors in exchange and liquidity operations.
  • Authorization mistakes and manipulation vulnerabilities.
  • Errors in swaps and reliance on price oracles.

• Centralization Risks:

  • Dependency on external contracts and administrator control.

UniV3LiquidityAmo:

• Systemic Risks:

  • Calculation errors and oracle dependency.
  • Unintended position changes due to mistakes.

• Centralization Risks:

  • Administrator control and reliance on external contracts.

RdpxV2Core:

• Systemic Risks:

  • Centralized control and oracle dependence (The oracle is out the scope in this contests, but we recommend to consider auditing and make code reviews in those contracts).
  • Delegation mechanism and AMO dependency.

• Centralization Risks:

  • Administrator control over pause and withdrawal.
  • Minting control and reliance on external contracts.

RdpxV2Bond:

Centralization Risks:

• Minting control and bond amount.
• Ownership and contract governance.

RdpxDecayingBonds:

• Systemic Risks:
  • Vulnerability in emergencyWithdraw() function could lead to fund loss during emergencies.
  • Improper implementation or malicious exploitation of pausing could disrupt contract functionality.
  • Interaction with external contracts introduces security and behavior risks.
  • Lack of mechanisms to limit bond supply may cause unintended inflation.

Centralization Risks:

• Admin's control over pausing and withdrawal might centralize power.
• Concentrated MINTER_ROLE control could lead to centralized bond issuance.
• Centralized RDPXV2CORE_ROLE control might impact bond adjustments.
• Absence of governance could centralize upgrade decisions.
• Initial deployer's role ownership could centralize contract control.

DpxEthToken:

• Systemic Risks:

  • Lack of burn limits and reliance on external contracts.

• Centralization Risks:

  • Administrator control and centralized role assignment.

PerpetualAtlanticVault:

• Systemic Risks:

  • Funding calculation errors and stale data.
  • External contract dependencies and reentrancy.

• Centralization Risks:

  • Manager role control and administrator authority.
  • Whitelist control and contract upgrades.

PerpetualAtlanticVaultLP:

• Systemic and Centralization Risks:
  • Centralized control due to dependency on perpetualAtlanticVault contract.
  • Vulnerabilities in external contracts and liquidity risk.

ReLPContract:

• Systemic Risks:

  • Slippage tolerance risk and role centralization.

• Centralization Risks:

  • Administrator control and dependence on external contracts.

These summaries outline the major risks that each contract is exposed to in relation to potential system-wide problems and concentration of control.

Dependency risk:

• We observed that old and unsafe versions of Openzeppelin are used in the project, this should be updated to the latest one:

    package.json#L4
    4: "version": "4.9.0",

• Latest is 4.9.3 (as of July 28, 2023), version used in project is 4.9.0
• You can read more of this issue in our QA.

6- New insights and learning from this audit

Our new insights from this Dopex rDPX V2 protocol audit have been exponential. We have learned about the intricate bonding mechanisms, including regular, delegate, and decaying bonds, which cater to various scenarios but require user understanding. Inspired by Frax Finance, Algorithmic Market Operations AMOs are integrated to mitigate risks. The addition of perpetual options and increased yield issuance for receipt token holders aims to incentivize participation. Governance-controlled parameters ensure adaptability, but effective governance is essential. The fee structure of the redemption mechanism impacts liquidity. The implementation strategy emphasizes initial liquidity and participation.

7- Test analysis

The audit scope of the contracts to be reviewed is 95%, with the aim of reaching 100%. However, the tests provided by the protocol are at a high level of comprehension, giving us the opportunity to easily conduct Proof of Concepts (PoCs) to test certain bugs present in the project.

How Could They Have Improved?

While the provided codes seems to be functional, there are always areas for improvement to ensure better security, efficiency, and readability in both the contracts and the protocol.

• Here are some potential areas for improvement:

  1. Modularization and Separation of Concerns:

     • The contract RdpxV2Core code is lengthy and contains many functions in a single unit. Consider separating different responsibilities into smaller, specific modules. For example, you can create modules for:
       • Bond handling
       • Delegate interaction
       • Price calculations etc.

     This will improve code readability and maintainability.

  2. Comments and Detailed Documentation:

     • While the code has comments, certain sections may necessitate more comprehensive explanations, such as the Execute function in the UniV3LiquidityAmo.sol contract.

     It is recommended to document it as per the instructions provided by the sponsor in the Discord DM:

     psytama — 22/08/2023 21:18
         - "We just added a generic proxy function in case an unforeseeable situation arises; we would utilize it"

     Enhance the clarity and comprehensibility of the code by incorporating explicit and elucidative comments for every function and segment. This practice will not only enhance understanding but also facilitate seamless maintenance.

  3. Structured Comments:

     • Use structured comments, such as standardized function headers (NATS), to describe the function, its parameters, returns, and behavior. This will make the code more understandable for other developers.

Conclusion

In general, the Dopex rDPX V2 project exhibits an interesting and well-developed architecture we believe the team has done a good job regarding the code, but the identified risks need to be addressed, and measures should be implemented to protect the protocol from potential malicious use cases. It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.

Time Spent ⏱

A total of 8 days were dedicated to completing this audit, distributed as follows:

1. 1st Day: Devoted to comprehending the protocol's functionalities and implementation.
2. 2nd Day: Initiated the analysis process, leveraging the documentation offered by the Dopex Protocol.
3. 3nd Day & 4rd Day: We dedicated ourselves to taking notes while reading through the contracts line by line.
4. 5th Day: From the notes taken, we decided what goes into the QA report and started it.
5. 6th Day: After finishing the QA, we started with some PoCs that we wanted to do to confirm our doubts about certain functions.
6. 7th Day: We focused on writing the reports for the medium findings we discovered.
7. 8rd Day: Focused on conducting a thorough analysis, incorporating meticulously crafted diagrams derived from the contracts and the note taken by us.

Time spent:

72 hours