c4-submissions commented 1 year ago

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L56

Vulnerability details

Impact

initialiseManager is defined as follows:

  function initializeManager() external {
    if (address(safeManager) == address(0)) _setSafeManager(msg.sender);
  }

The condition if (address(safeManager) == address(0)), implies that once safeManager has been initialised it cannot be changed again by calling initializeManager. Note that the function can be called by anyone.

Also, in ODSafeManager, the constructor is defined as follows:

  constructor(address _safeEngine, address _vault721) {
    safeEngine = _safeEngine.assertNonNull();
    vault721 = IVault721(_vault721);
    vault721.initializeManager();
  }

This implies that Vault721 has been deployed already but the function initializeManager has not been called before. It is only called in the constructor here. This creates an opportunity for a malicious actor to front-run the deployment of ODSafeManager. The malicious actor would call the initializeManager before the deployment of ODSafeManager by paying a higher gas fee. Doing so would fail the deployment of ODSafeManager as initializeManager can only be called once.

Proof of Concept

initialiseManager:

  function initializeManager() external {
    if (address(safeManager) == address(0)) _setSafeManager(msg.sender);
  }

ODSafeManager:

  constructor(address _safeEngine, address _vault721) {
    safeEngine = _safeEngine.assertNonNull();
    vault721 = IVault721(_vault721);
    vault721.initializeManager();
  }

Tools Used

Manual review

Recommended Mitigation Steps

There should be a modifier in the initializeManager function that should only be called by selected actors.

Assessed type

Access Control

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 duplicate of #16

c4-judge commented 1 year ago

MiloTruck changed the severity to QA (Quality Assurance)