code-423n4 / 2023-09-ondo-findings

7 stars 5 forks source link

Incorrect initialization of `rUSDY.sol` #532

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L109-L147

Vulnerability details

Impact

rUSDY.sol contract inherits PausableUpgradeable contract but does not invoke its initialzers during its own initialization. Due to which the state of PausableUpgradeable contract remain uninitialized.

File: contracts/usdy/rUSDY.sol

contract rUSDY is
  Initializable,
  ContextUpgradeable,
  PausableUpgradeable,

  // some code
File: contracts/usdy/rUSDY.sol

  function initialize(
    address blocklist,
    address allowlist,
    address sanctionsList,
    address _usdy,
    address guardian,
    address _oracle
  ) public virtual initializer {
    __rUSDY_init(blocklist, allowlist, sanctionsList, _usdy, guardian, _oracle);
  }

  function __rUSDY_init(
    address blocklist,
    address allowlist,
    address sanctionsList,
    address _usdy,
    address guardian,
    address _oracle
  ) internal onlyInitializing {
    __BlocklistClientInitializable_init(blocklist);
    __AllowlistClientInitializable_init(allowlist);
    __SanctionsListClientInitializable_init(sanctionsList);
    __rUSDY_init_unchained(_usdy, guardian, _oracle);
   }

PausableUpgradeable initialization is very much important and it looks as below,

File: contracts/security/PausableUpgradeable.sol

    function __Pausable_init() internal onlyInitializing {
        __Pausable_init_unchained();
    }

    function __Pausable_init_unchained() internal onlyInitializing {
        _paused = false;
    }

This initializes the contract in unpaused state. Due to this _paused state is not initialized. This breaks the overall functionality with respect to PausableUpgradeable.sol. Openzeppelin reference can be checked here

It is to be noted that automated report has made this issue low severity and has some false positives too. The automated report does not mention the impact of non-initialization of PausableUpgradeable but it must be noted PausableUpgradeable intialization is very much required otherwise the contract will not be initialized in unpaused state.

Proof of Concept

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/usdy/rUSDY.sol#L109-L147

Tools Used

Manual review

Recommended Mitigation Steps

Consider initializing the PausableUpgradeable in rUSDY.sol initialize() function.

For example for understanding:


    function initialize(

      // some code

    ) public initializer {
+        __Pausable_init();

      // some code

    }

Assessed type

Other

raymondfam commented 1 year ago

Uninitialized _paused is false by default. QA at best.

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #47

c4-judge commented 1 year ago

kirk-baird marked the issue as unsatisfactory: Invalid