c4-submissions commented 10 months ago

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L126-L128 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L172-L174 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/ODSafeManager.sol#L118-L133 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L94-L99

Vulnerability details

Impact

Vault721 contract is the ERC721 "Non-fungible Vault" (NFV) that manages all SAFEs ownership, transfers, and approvals via the ERC721 standard, the minter of the NFV is the ODSafeManager contract.
ODSafeManager contract acts as interface to the SAFEEngine to facilitate the management of SAFEs; such as opening safes,modifying SAFE collateral, moving SAFE collateral,etc...
When a user opens a SAFE via ODSafeManager.openSAFE function; a NFV will be minted to the user where it will have the same id as the SAFE id so that the ids of any of them can be used interchangeably in the system, and the _safeId is the state variable that tracks the number of the opened safes/and minted NFV by the ODSafeManager contract.
The initial value of the _safeId is zero; and it will be incremented with each opened SAFE.
But a problem will be caused if the governance change the ODSafeManager address via Vault721.setSafeManager; as this will result in adding a new deployed safe manager where it will have the _safeId set to zero, which will result in breaking the synchronization between the _safeId and the last minted NFV id (breaking an invariant).
But how could this be a problem? well, this will result in disabling SAFE opening and NFV minting:
1. The old safe manager contract has a _safeId of 1000, and this matches the id of the last minted NFV, so the next NFV id supposed to be 1001, and that should match the value of _safeId.
2. The governance decided to change the safe manager address (with a new deployed one) with extra/updated features; so the _safeId of the new ODSafeManager will be zero.
3. Now a user tries to open a SAFE; so the _safeId will be incremeted by 1 (and will equal one) and when the Vault721.mint is called to mint a NFV; the function will revert as it's requested to mint a NFV with id=_safeId=1, and this NFV has been already deployed by the old safe manager contract.

Proof of Concept

Vault721.setSafeManager function

  function setSafeManager(address _safeManager) external onlyGovernor {
    _setSafeManager(_safeManager);
  }

Vault721._setSafeManager function

  function _setSafeManager(address _safeManager) internal nonZero(_safeManager) {
    safeManager = IODSafeManager(_safeManager);
  }

ODSafeManager.openSAFE function

  function openSAFE(bytes32 _cType, address _usr) external returns (uint256 _id) {
   //some code...

    ++_safeId;

   //some code...

    vault721.mint(_usr, _safeId);

    //some code...
  }

Vault721.mint function

function mint(address _proxy, uint256 _safeId) external {
    require(msg.sender == address(safeManager), 'V721: only safeManager');
    require(_proxyRegistry[_proxy] != address(0), 'V721: non-native proxy');
    address _user = _proxyRegistry[_proxy];
    _safeMint(_user, _safeId);
  }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Add a mechanism in the ODSafeManger to initialize the _safeId based on the total number of previously minted NFV + 1 (another variable to be added to the Vault721 to track the total number of minted NFVs).

Assessed type

Context

c4-pre-sort commented 10 months ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #37