Open Dollar - hals's results

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

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/BasicActions.sol#L94-L115 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/BasicActions.sol#L31-L47

Vulnerability details

Impact

• When the user generates a debt against his collateral (when he locks a collateral first) in BasicActions.generateDebt:

  • First the user requests a debt of a value _deltaWad.

  • Then this requested value is modified by the _getGeneratedDeltaDebt function: where the requested debt by the user is updated by comparing the coin balance of the SAFE (which is minted when the user generates a debt) with the requested debt; if the requested debt exceeds the coin balance of the SAFE; the requested debt value is modified by calculating the needed deltaDebt that would be enough to exit wad amount of COIN tokens with the current coins balance of the SAFE.

  • Then a debt is going to be generated for the SAFE with this modified debt value (by updating the SAFE's collateralization by the SAFEEngine):

    _modifySAFECollateralization(
          _manager,
          _safeId,
          0,
          _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
        );

  • Then the user (EOA) will be minted system tokens of a value equals to the original value requested by him not the updated value generated by the _getGeneratedDeltaDebt function, and this is done via _exitSystemCoins function which will in turn call the __coinJoin.exit function:

    _exitSystemCoins(_coinJoin, _deltaWad * RAY);

• So as can be seen; the values doesn't match; as the modified _deltaDebt that is used to update the safe.generatedDebt (via BasicActions._modifySAFECollateralization function) might be of a value lesser/larger (depending on the rate and coins balance of the safe) than the actual minted tokens; so when the user wants to repay the SAFE debt his minted tokens might not be enough to do so.

Proof of Concept

BasicActions._generateDebt function

function _generateDebt(
    address _manager,
    address _taxCollector,
    address _coinJoin,
    uint256 _safeId,
    uint256 _deltaWad
  ) internal {
    address _safeEngine = ODSafeManager(_manager).safeEngine();
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);
    ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);

    // Generates debt in the SAFE
    _modifySAFECollateralization(
      _manager,
      _safeId,
      0,
      _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad) //  @audit : will not equal the input _deltaWad
    );

    // Moves the COIN amount to user's address
    _collectAndExitCoins(_manager, _coinJoin, _safeId, _deltaWad);// @audit : _deltaWad is different from the one used to generate dabt in safeEngine
  }

BasicActions._getGeneratedDeltaDebt function

  function _getGeneratedDeltaDebt(
    address _safeEngine,
    bytes32 _cType,
    address _safeHandler,
    uint256 _deltaWad
  ) internal view returns (int256 _deltaDebt) {
    uint256 _rate = ISAFEEngine(_safeEngine).cData(_cType).accumulatedRate;
    uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_safeHandler);

    // If there was already enough COIN in the safeEngine balance, just exits it without adding more debt
    if (_coinAmount < _deltaWad * RAY) {
      // Calculates the needed deltaDebt so together with the existing coins in the safeEngine is enough to exit wad amount of COIN tokens
      _deltaDebt = ((_deltaWad * RAY - _coinAmount) / _rate).toInt();
      // This is neeeded due lack of precision. It might need to sum an extra deltaDebt wei (for the given COIN wad amount)
      _deltaDebt = uint256(_deltaDebt) * _rate < _deltaWad * RAY ? _deltaDebt + 1 : _deltaDebt;
    }
  }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Update _generateDebt function to use the same modified _deltaWad value for generating debt and minting system tokens:

function _generateDebt(
    address _manager,
    address _taxCollector,
    address _coinJoin,
    uint256 _safeId,
    uint256 _deltaWad
  ) internal {
    address _safeEngine = ODSafeManager(_manager).safeEngine();
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);
    ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);

+   int256 _UpdatedDeltaDebt=_getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad);

    // Generates debt in the SAFE
    _modifySAFECollateralization(
      _manager,
      _safeId,
      0,
-     _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
+     _UpdatedDeltaDebt
    );

    // Moves the COIN amount to user's address
-   _collectAndExitCoins(_manager, _coinJoin, _safeId, _deltaWad);
+   _collectAndExitCoins(_manager, _coinJoin, _safeId, _UpdatedDeltaDebt);
  }

Assessed type

Context

Judge has assessed an item in Issue #385 as 2 risk. The relevant finding follows:

[L-03] UniV3Relayer contract works only with tokens of decimals <= 18 Details When the UniV3Relayer contract is deployed; the multiplier state variable that’s going to be used to parse the price result from the aggregator is calculated based on a maximum _quoteToken decimals of 18:

multiplier = 18 - IERC20Metadata(_quoteToken).decimals(); So if any _quoteToken with decimals > 18 will not be supported by the UniV3Relayer contract, and this will limit the number of tokens that can be integrated in/adopted by the protocol.

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/gov/ODGovernor.sol#L41-L41 https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/gov/ODGovernor.sol#L85-L92

Vulnerability details

Impact

• The ODGovernor contract inherits the utility GovernorSettings contract from openzeppelin, and the GovernorSettings is initialized in the constructor with initialVotingDelay, initialVotingPeriod & initialProposalThreshold measured in blocks, and these values where set to (1, 15, 0) respectively:

  GovernorSettings(1, 15, 0)

• So the proposal threshold is set initially to zero, which technically means that anyone with zero voting weight that doesn't have any voting tokens will be able to propose, and this will introduce a serious risk to the system:

1. Any malicious actor can add malicious proposals to the system, and these proposals might pass the voting threshold and get executed , And as there's no exposed function to cancel proposals; any malicious proposal can be voted on and executed and harming the protocol integrity.

2. Proposals creation can be front-run: any malicious actor can observe the mempool and whenever there's a proposal being added to the system; the malicious actor will front-runs it by adding a proposal with the same arguments (which will result in the same proposal hash), and he can do so since the proposalThreshold is set to zero and the check for proposer votes will be passed, here's the check made by the @openzeppelin/governance/Governor.sol.propose function:

   require(
               getVotes(_msgSender(), block.number - 1) >= proposalThreshold(),
               "Governor: proposer votes below proposal threshold"
           );

This will block adding proposals to the system by malicious actors front-running the ODGovernor.propose transactions.

Proof of Concept

Code Instances:

ODGovernor.constructor

GovernorSettings(1, 15, 0)

ODGovernor.propose function

  function propose(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    string memory description
  ) public override(Governor, GovernorCompatibilityBravo, IGovernor) returns (uint256) {
    return super.propose(targets, values, calldatas, description);
  }

[Governor.propose function from (@openzeppelin/governance/Governor.sol) where the check is made on the proposer votes against threshold]

 require(
            getVotes(_msgSender(), block.number - 1) >= proposalThreshold(),
            "Governor: proposer votes below proposal threshold"
        );

Foundry PoC:

1. Add the following ODGovernorTest test file to the following directory od-contracts/test/unit/ where any user with zero voting weight can add proposals:

   // SPDX-License-Identifier: GPL-3.0
   pragma solidity 0.8.19;
   
   import "forge-std/Test.sol";
   import {ODGovernor} from "@contracts/gov/ODGovernor.sol";
   import {TimelockController} from "@openzeppelin/governance/TimelockController.sol";
   import {IGovernor} from "@openzeppelin/governance/IGovernor.sol";
   import {ERC20} from "@openzeppelin/token/ERC20/ERC20.sol";
   
   contract MockVotingToken is ERC20 {
       constructor() ERC20("VotingToken", "VT") {}
   
       // this function mocks the ERC20Votes.getPastVotes; and it's set here to zero to test that the zero voting weight (doesn't have votingTokens) will be able to propose
       function getPastVotes(
           address account,
           uint256 blockNumber
       ) public view returns (uint256) {
           return 0;
       }
   }
   
   contract ODGovernorTest is Test {
       address deployer = address(0x10);
       address user = address(0x20);
       address randomUser = address(0x30);
       ODGovernor governorContract;
   
       function test_Anyone_Can_Propose() public {
           //1. deployer deploys the ODGovernor contract:
           vm.startPrank(deployer);
           //------deploy the required Timelock contract:
           address[] memory proposers;
           address[] memory executors;
           TimelockController timelock = new TimelockController(
               0,
               proposers,
               executors,
               deployer
           );
   
           //------deploy the votingToken and the governorContract :
           MockVotingToken votingToken = new MockVotingToken();
           governorContract = new ODGovernor(address(votingToken), timelock);
           vm.stopPrank();
   
           assertEq(governorContract.proposalThreshold(), 0); // the contract is deployed with zero proposal threshold (this value is set in the constructor)
   
           //2. now any random user that doesn't have any votingToken will be able to add a proposal:
   
           //------proposal setup for testing:
           address[] memory targets = new address[](1);
           targets[0] = randomUser;
   
           uint256[] memory values = new uint256[](1);
           values[0] = 1 ether;
   
           bytes[] memory calldatas = new bytes[](1);
           calldatas[0] = "test proposal";
           string memory description;
   
           //------now the random user which has zero voting weight will be able to add a proposal:
           vm.startPrank(randomUser);
           assertEq(votingToken.balanceOf(randomUser), 0);
   
           uint256 proposalId;
           assertEq(proposalId, 0);
   
           proposalId = governorContract.propose(
               targets,
               values,
               calldatas,
               description
           );
   
           assertNotEq(proposalId, 0); //now the  has been created
   
           assertTrue(
               governorContract.state(proposalId) ==
                   IGovernor.ProposalState.Pending,
               "proposal created"
           );
       }
   }

2. Test result:

   $ forge test --match-test test_Anyone_Can_Propose -vvvv
   Running 1 test for test/unit/ODGovernorTest.sol:ODGovernorTest
   [PASS] test_Anyone_Can_Propose() (gas: 7845563)
   Traces:
     [7845563] ODGovernorTest::test_Anyone_Can_Propose()
       ├─ [0] VM::startPrank(0x0000000000000000000000000000000000000010)
       │   └─ ← ()
       ├─ [2022894] → new TimelockController@0xFb64EadC5e4Def65C63A9eA66Ad76545E9FcCFbc
       │   ├─ emit RoleAdminChanged(role: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5, previousAdminRole: 0x0000000000000000000000000000000000000000000000000000000000000000, newAdminRole: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5)
       │   ├─ emit RoleAdminChanged(role: 0xb09aa5aeb3702cfd50b6b62bc4532604938f21248a27a1d5ca736082b6819cc1, previousAdminRole: 0x0000000000000000000000000000000000000000000000000000000000000000, newAdminRole: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5)
       │   ├─ emit RoleAdminChanged(role: 0xd8aa0f3194971a2a116679f7c2090f6939c8d4e01a2a8d7e41d55e5351469e63, previousAdminRole: 0x0000000000000000000000000000000000000000000000000000000000000000, newAdminRole: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5)
       │   ├─ emit RoleAdminChanged(role: 0xfd643c72710c63c0180259aba6b2d05451e3591a24e58b62239378085726f783, previousAdminRole: 0x0000000000000000000000000000000000000000000000000000000000000000, newAdminRole: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5)
       │   ├─ emit RoleGranted(role: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5, account: TimelockController: [0xFb64EadC5e4Def65C63A9eA66Ad76545E9FcCFbc], sender: 0x0000000000000000000000000000000000000010)    │   ├─ emit RoleGranted(role: 0x5f58e3a2316349923ce3780f8d587db2d72378aed66a8261c916544fa6846ca5, account: 0x0000000000000000000000000000000000000010, sender: 0x0000000000000000000000000000000000000010)
       │   ├─ emit MinDelayChange(oldDuration: 0, newDuration: 0)
       │   └─ ← 9351 bytes of code
       ├─ [620949] → new MockVotingToken@0xb507F3e536FddFC13bA355224a4c6d4e69B25Bb9
       │   └─ ← 2876 bytes of code
       ├─ [4792515] → new ODGovernor@0xCAf5600Cca91bbfcc2504524c4c73972e3437Cf6
       │   ├─ emit VotingDelaySet(oldVotingDelay: 0, newVotingDelay: 1)
       │   ├─ emit VotingPeriodSet(oldVotingPeriod: 0, newVotingPeriod: 15)
       │   ├─ emit ProposalThresholdSet(oldProposalThreshold: 0, newProposalThreshold: 0)
       │   ├─ emit QuorumNumeratorUpdated(oldQuorumNumerator: 0, newQuorumNumerator: 3)
       │   ├─ emit TimelockChange(oldTimelock: 0x0000000000000000000000000000000000000000, newTimelock: TimelockController: [0xFb64EadC5e4Def65C63A9eA66Ad76545E9FcCFbc])
       │   └─ ← 23204 bytes of code
       ├─ [0] VM::stopPrank()
       │   └─ ← ()
       ├─ [398] ODGovernor::proposalThreshold() [staticcall]
       │   └─ ← 0
       ├─ [0] VM::startPrank(0x0000000000000000000000000000000000000030)
       │   └─ ← ()
       ├─ [2561] MockVotingToken::balanceOf(0x0000000000000000000000000000000000000030) [staticcall]
       │   └─ ← 0
       ├─ [264539] ODGovernor::propose([0x0000000000000000000000000000000000000030], [1000000000000000000 [1e18]], [0x746573742070726f706f73616c], )
       │   ├─ [392] MockVotingToken::getPastVotes(0x0000000000000000000000000000000000000030, 0) [staticcall]
       │   │   └─ ← 0
       │   ├─ emit ProposalCreated(proposalId: 91543011429934823020049420064945031363908323350090815725489190674033666692309 [9.154e76], proposer: 0x0000000000000000000000000000000000000030, targets: [0x0000000000000000000000000000000000000030], values: [1000000000000000000 [1e18]], signatures: [], calldatas: [0x746573742070726f706f73616c], startBlock: 2, endBlock: 17, description: )
       │   └─ ← 91543011429934823020049420064945031363908323350090815725489190674033666692309 [9.154e76]
       ├─ [3209] ODGovernor::state(91543011429934823020049420064945031363908323350090815725489190674033666692309 [9.154e76]) [staticcall]
       │   └─ ← 0
       └─ ← ()
   
   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.07ms
   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Testing & Foundry.

Recommended Mitigation Steps

Add a reasonable value > 0 for the initialProposalThreshold when deploying the ODGovernor contract:

  constructor(
    address _token,
    TimelockController _timelock
  )
    Governor('ODGovernor')
-   GovernorSettings(1, 15, 0)
+   GovernorSettings(1, 15, 10) //10 is an example; should be any value > 0
    GovernorVotes(IVotes(_token))
    GovernorVotesQuorumFraction(3)
    GovernorTimelockControl(_timelock)
  {}

Assessed type

Governance

Judge has assessed an item in Issue #385 as 2 risk. The relevant finding follows:

[L-04] ODSafeManager.allowSAFE function enables any allowed address to add/remove other allowed addresses Details The ODSafeManager.allowSAFE function is meant by design to allow/disallow any address to manage the safe; this can be first accessed by the SAFE owner to add an allowed address; then this allowed address will have the ability to add/remove (allow/disallow) any address on the SAFE.

As the allowed addresses on the safe can call vital functions on the SAFE such as transferring its collateral and internal coins;any malicious address can overtake the SAFE and drain it.

So it's recommended to enable the safe owner only from accessing the ODSafeManager.allowSAFE function on his own SAFE.

Proof of Concept ODSafeManager.allowSAFE function

function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) { address _owner = _safeData[_safe].owner; safeCan[_owner][_safe][_usr] = _ok; emit AllowSAFE(msg.sender, _safe, _usr, _ok); } Recommended Mitigation Steps Update ODSafeManager.allowSAFE function to enable the SAFE owner only from allowing/disallowing any address on the SAFE (similar to the design of allowHandler function).

Findings Summary

Low

[L-01] Setting the addresses of surplusAuctionHouse & debtAuctionHouse to zero will require redeploying the AccountingEngine contract <a id="l-01"></a>

Details

• When deploying the AccountingEngine contract: if surplusAuctionHouse & debtAuctionHouse addresses are both zero address; they can never be re-set again due to the _validateParameters function that is invoked in the validParams modifier after the contract manager calls the inherited modifyParameters function.

• The modifyParameters function will allow setting one value only with each call; then the _validateParameters checks if neither of the surplusAuctionHouse or the debtAuctionHouse is a zero address, if anyone of them is set to zero; the function will revert.

• So setting these addresses to zero in the constructor upon deployment will require a redeployemnt of the AccountingEngine contract as these addresses can't be re-set again .

Proof of Concept

AccountingEngine._validateParameters function

  function _validateParameters() internal view override {
    address(surplusAuctionHouse).assertNonNull();
    address(debtAuctionHouse).assertNonNull();
  }

AccountingEngine.modifyParameters function

  function modifyParameters(bytes32 _param, bytes memory _data) external isAuthorized validParams {
    _modifyParameters(_param, _data);
    emit ModifyParameters(_param, _GLOBAL_PARAM, _data);
  }

AccountingEngine.validParams modifier

  modifier validParams() {
    _;
    _validateParameters();
  }

Recommended Mitigation Steps

In the AccountingEngine.constructor: check that neither of the assigned addresses are set equal t the zero address.

[L-02] The governance will still have power over the AccountingEngine contract while it shouldn't as per the intended design <a id="l-02" ></a>

Details

• In AccountingEngine contract: the contract deployer (the manager) can add authorize any address to have access on the privilaged functions of the contract by calling addAuthorization function.

• It was mentioned in the protocol documents that the governance doesn't have power over the contract (not an authorized entity), this is stated in the contracts docs as a comment on the transferPostSettlementSurplus function:

  transferPostSettlementSurplus Transfer any remaining surplus after the disable cooldown has passed. Meant to be a backup in case GlobalSettlement.processSAFE has a bug, governance doesn't have power over the system and there's still surplus left in the AccountingEngine which then blocks GlobalSettlement.setOutstandingCoinSupply.

• But the contract manager can authorize any address without checking if it's the governance address or not; thus breaking th eintended design.

Proof of Concept

AccountingEngine.addAuthorization function

  function addAuthorization(address _account) external override(Authorizable, IAuthorizable) isAuthorized whenEnabled {
    _addAuthorization(_account);
  }

Recommended Mitigation Steps

Add acheck in the addAuthorization function to prevent granting the governance address authorization over AccountingEngine contract.

[L-03] UniV3Relayer contract works only with tokens of decimals <= 18 <a id="l-03" ></a>

Details

• When the UniV3Relayer contract is deployed; the multiplier state variable that's going to be used to parse the price result from the aggregator is calculated based on a maximum _quoteToken decimals of 18:

  multiplier = 18 - IERC20Metadata(_quoteToken).decimals();

• So if any _quoteToken with decimals > 18 will not be supported by the UniV3Relayer contract, and this will limit the number of tokens that can be integrated in/adopted by the protocol.

Proof of Concept

UniV3Relayer.constructor

multiplier = 18 - IERC20Metadata(_quoteToken).decimals();

Recommended Mitigation Steps

Modify the multiplier variable calculation to account for _quoteTokens of decimals > 0.

[L-04] ODSafeManager.allowSAFE function enables any allowed address to add/remove other allowed addresses<a id="l-04" ></a>

Details

• The ODSafeManager.allowSAFE function is meant by design to allow/disallow any address to manage the safe; this can be first accessed by the SAFE owner to add an allowed address; then this allowed address will have the ability to add/remove (allow/disallow) any address on the SAFE.

• As the allowed addresses on the safe can call vital functions on the SAFE such as transferring its collateral and internal coins;any malicious address can overtake the SAFE and drain it.

• So it's recommended to enable the safe owner only from accessing the ODSafeManager.allowSAFE function on his own SAFE.

Proof of Concept

ODSafeManager.allowSAFE function

  function allowSAFE(uint256 _safe, address _usr, uint256 _ok) external safeAllowed(_safe) {
    address _owner = _safeData[_safe].owner;
    safeCan[_owner][_safe][_usr] = _ok;
    emit AllowSAFE(msg.sender, _safe, _usr, _ok);
  }

Recommended Mitigation Steps

Update ODSafeManager.allowSAFE function to enable the SAFE owner only from allowing/disallowing any address on the SAFE (similar to the design of allowHandler function).