Ability to use emergencyWithdraw in RdpxDecayingBonds.sol may raise red flags for Investors

[code423n4]

Lines of code

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

Vulnerability details

Impact

emergencyWithdraw allows to withdraw all the existing tokens from the contract by users with DEFAULT_ADMIN_ROLE role.

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 report 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 (malicious admin can withdraw tokens whenever he/she wants), 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

Proof of Concept

File: contracts/decaying-bonds/RdpxDecayingBonds.sol

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

While calling emergencyWithdraw by user with DEFAULT_ADMIN_ROLE, every token inside the contract will be sent to msg.sender. This function should be considered as rug-pull vector, since it allows to drain the whole contract's balance. Moreover, in a contrary to what the documentation of this function says:

File: contracts/decaying-bonds/RdpxDecayingBonds.sol

* @dev    Can only be called by the owner

not only the owner of the contract, but everyone with DEFAULT_ADMIN_ROLE can call this function. Since RdpxDecayingBonds inherits from AccessControl - there's a grantRole() function available which allows to create more than one user with DEFAULT_ADMIN_ROLE role. This makes this function to violate its documentation, since more than just a contract's owner can call emergencyWithdraw - everyone with DEFAULT_ADMIN_ROLE can call this function. 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

Multi Sig Address should be hardcoded into the contract to ensure the fund move to a safe wallet whenever emergencyWithdraw needs to be called.
Moreover, 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 emergencyWithdraw can rug-pull the whole contract balance.

• 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]

Centralization risk. QA at best.

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #2031

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope