Potential rug-pull vector by changing Oracle address

[code423n4]

Lines of code

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

Vulnerability details

Impact

setPriceingOracleAddress allows to change the address of the oracles. By setting a malicious oracles, the price manipulation is possible.

Malicious admin can change the address of the oracle to manipulate the current prices of the assets in the protocol.
While rug-pull vectors which straightforwardly allows to withdraw all the funds from the protocol are nothing new in the DeFi ecosystem - this attack vector is slightly different. Instead of withdrawing the funds, malicious admin manipulates the price of the assets inside the protocol - to his advantage. One of the example of this attack vector happened very recently: 25th August 2023 - https://twitter.com/PeckShieldAlert/status/1694946221788729440 . Thus I've decided to report this issue as separate finding and evaluated it as Medium.

Even if the DEFAULT_ADMIN_ROLE is trustworthy, having a rug-vector in the contract may have a negative impact on the protocol's reputation. Multiple online tools reports contract with rug-pulls vector as a high-risk: (e.g.: https://twitter.com/RugDocIO/status/1411732108029181960). Even this function had been created with good intentions, it's often associate with rug pull, especially that rug pulls are not uncommon in DeFi. Rug pull vectors increases a risk for investors to lose their hard earn money, thus it has a negative impact on the smart contract's reputation.

Similar findings which had been classified at Code4rena as Medium:

• TIMELOCK_ROLE Has Absolute Power to Withdraw All FUND May Raise Red Flags for Investors 2021-11-malt-findings#125
• Owner or Managers can rug Aave rewards 2022-04-pooltogether-findings#89

Having this function available is not only a huge red flag for the Investors, but it is also huge centralization risk.
The DEFAULT_ADMIN_ROLE is being set in constructor and it's a single EOA.

Having a single EOA as a single DEFAULT_ADMIN_ROLE role is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary.

User with DEFAULT_ADMIN_ROLE role holds a lot of power within the contract. Compromise of account with this role puts the contract's integrity at risk.

The very similar issue had been evaluated during a previous Code4rena audit as an issue with Medium severity:

• Stader OPERATOR is single point of failure 2023-06-stader-findings#344
• Centralization risk: admin have privileges: admin can set address to mint any amount of frxETH, can set any address as validator, and change important state in frxETHMinter and withdraw fund from frcETHMinter  2022-09-frax-findings#107

Moreover, it's crucial to notice, that such a important function - as setPriceingOracleAddress does not implement any timelock mechanism, which would give protocol's users a change to properly react in case of emergency.

Proof of Concept

File: contracts/core/RdpxV2Core.sol

  function setPricingOracleAddresses(
    address _rdpxPriceOracle,
    address _dpxEthPriceOracle
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_rdpxPriceOracle != address(0), 17);
    _validate(_dpxEthPriceOracle != address(0), 17);

    pricingOracleAddresses = PricingOracleAddresses({
      rdpxPriceOracle: _rdpxPriceOracle,
      dpxEthPriceOracle: _dpxEthPriceOracle
    });

    emit LogSetPricingOracleAddresses(pricingOracleAddresses);
  }

Since RdpxV2Core inherits from AccessControl - there's a grantRole() function available which allows to create more than one user with DEFAULT_ADMIN_ROLE role. This increases the risk, since in a group of accounts (with DEFAULT_ADMIN_ROLE role), the more users, the higher the risk is that one of them will become malicious or will become a victim to a hack (e.g. got their private key stolen) - due to previously mentioned centralization risk.

Tools Used

Manual code review

Recommended Mitigation Steps

Critical admin functions should be protected by timelock - to give users a chance to react when something malicious would be happening.

Severity Evaluation

• Impact - High
  Users funds are being at risk as setPricingOracleAddresses can set a malicious oracles' addresses to manipulate the price of assets.

• Likelihood - Low
  The attack vector is available for DEFAULT_ADMIN_ROLE. However, due to centralization risk, and the ability to create multiple of admins by grantRole function (more admins means that it's more likely that one of them may become malicious) the risk should be consider as non-negligible.

Assessed type

Rug-Pull

[bytes032]

Over inflated

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)