Open Dollar - 0xhacksmithh's results

Lines of code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/CamelotRelayer.sol#L20 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/oracles/UniV3Relayer.sol#L18

Vulnerability details

Impact

refer below section.

Proof of Concept

As we can see in both contract GOERLI_ version of Factory address used In deployed Codebase, which give in wrong result.

So pair address and pool address always going to be wrong in both Relayers.

And those state variables are CONSTANT so it would not be changed cause in redeployment of code base.

import {CAMELOT_V3_FACTORY, GOERLI_CAMELOT_V3_FACTORY} from '@script/Registry.s.sol';
contract UniV3Relayer is IBaseOracle, IUniV3Relayer {
  // --- Registry ---
  address internal constant _UNI_V3_FACTORY = GOERLI_UNISWAP_V3_FACTORY;

import {UNISWAP_V3_FACTORY, GOERLI_UNISWAP_V3_FACTORY} from '@script/Registry.s.sol';

contract UniV3Relayer is IBaseOracle, IUniV3Relayer {
  // --- Registry ---
  address internal constant _UNI_V3_FACTORY = GOERLI_UNISWAP_V3_FACTORY;

Tools Used

Manual Review

Recommended Mitigation Steps

Use correct factory contract address

Assessed type

Oracle

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

[Low-01] When a User-1 sell/transfer a safe to User-2, during transfer allowance is not clear in case of User-1 safeCan is a mapping which set allowance for other addresses, by which they can perform action on behalf of owner.

mapping(address _owner => mapping(uint256 _safeId => mapping(address _caller => uint256 _ok))) public safeCan; So basically when owner(user-1) sell/transfer his safe to other(user-2) these allowed address never clear. Problem occurs when same Owner(user-1) again buy that safe from User-2 again after some time period, Now previously allowed addresses now back in form(active) which may be the Owner(User-1) not aware off.

For a instance consider a senario Bob has a Safe with id 99 Bob give allowance via safeCan to Sam Bob sell safe to Alice Years passed Alice sell safe(id-99) to Bob Now Sam has access to Safe, which may be Bob not aware of. Now he can preform action behalf of Bob

Code https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L49-L53 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L39

Mitigation Re-think on current senario

[Low-01] When a User-1 sell/transfer a safe to User-2, during transfer allowance is not clear in case of User-1

safeCan is a mapping which set allowance for other addresses, by which they can perform action on behalf of owner.

mapping(address _owner => mapping(uint256 _safeId => mapping(address _caller => uint256 _ok))) public safeCan;

So basically when owner(user-1) sell/transfer his safe to other(user-2) these allowed address never clear. Problem occurs when same Owner(user-1) again buy that safe from User-2 again after some time period, Now previously allowed addresses now back in form(active) which may be the Owner(User-1) not aware off.

For a instance consider a senario Bob has a Safe with id 99 Bob give allowance via safeCan to Sam Bob sell safe to Alice Years passed Alice sell safe(id-99) to Bob Now Sam has access to Safe, which may be Bob not aware of. Now he can preform action behalf of Bob

Code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L49-L53 https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L39

Mitigation

Re-think on current senario

[Low-02] A mallicious allowed address can front run owner of a safe everytime and can give allowance for safe to other addresses.

safeAllowed is a modifier which ensure Owner of a perticular safe or allowed addresses by owner can perform perticular actions on safe.

  modifier safeAllowed(uint256 _safe) {
    address _owner = _safeData[_safe].owner;
    if (msg.sender != _owner && safeCan[_owner][_safe][msg.sender] == 0) revert SafeNotAllowed(); 
    _;
  }

And these allowance set via allowSAFE() which called by Owner or allowed addresses only

  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); 
  }

Consider senario A mallicious address get allownce for any xyz reson, and try to do some fishy activity Owner notice that and try to revoke allowance for that malicious address Hacker notice that, and frontrun Owner Tx and give allowance to a other address controlled by him Now he can continue doing phisy activity with new address Same thing happens when owner try to revoke other More Attacker can give allowance to so many milicious addresses

Code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L105-L109

Mitigation

allowSAFE() should only a onlyOwner function. Owner should be superior

[Low-03] allowSAFE using wrong parameters during event emission.

While setting allowance for address for a safe function emits event AllowSAFE which may be used in off-chain tracking/activity

While emiting it uses 4 parameters msg.sender, _safe, _usr, _ok which i believe stands for following msg.sender = Owner of safe _safe = Safe-id _user = Address to which allowance/revoke allowance occue ok = Allowance status of that address

But This allowSAFE function can callable by Owner as well as previoully allowed User

  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); 
  }

So When allowed User call this function, in that case msg.sender != Owner

So event always should emited as following

  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); 

+   emit AllowSAFE(_owner, _safe, _usr, _ok);
  }

Code

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L105-L109

[Low-4] initializeRenderer() and initializeManager() Both function has no access controls and they set critical state variable nftRender and safeManager respectively. So these could front-runnable

  function initializeManager() external { // @audit front-runnable
    if (address(safeManager) == address(0)) _setSafeManager(msg.sender);
  }

  /**
   * @dev initializes NFTRenderer contract
   */
  function initializeRenderer() external { // @audit
    if (address(nftRenderer) == address(0)) _setNftRenderer(msg.sender);
  }

No access control present Attacker can front-run and set those variable to own address So It will cause to deploy whole contract again.

Code

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

Mitigation

Apply some owner access control on these function, or made those private or internal and call them only in constructor

[Gas-01] Use delete instead of setting variables to their default value

  function popDebtFromQueue(uint256 _debtBlockTimestamp) external {
    if (block.timestamp < _debtBlockTimestamp + _params.popDebtDelay) revert AccEng_PopDebtCooldown();

    uint256 _debtBlock = debtQueue[_debtBlockTimestamp];

    if (_debtBlock == 0) revert AccEng_NullAmount();

    totalQueuedDebt = totalQueuedDebt - _debtBlock;
-   debtQueue[_debtBlockTimestamp] = 0;

+   delete debtQueue[_debtBlockTimestamp];

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L131

  function _onContractDisable() internal override { // @audit-ok
    totalQueuedDebt = 0; // @audit
    totalOnAuctionDebt = 0;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L247-L249

[Gas-02] Store state variable in stack memory and use it further [Here i mention those instances which are missing in Bot Report]

_params.debtAuctionBidSize and _params.debtAuctionBidSize could stored in memory and used further.

  function auctionDebt() external returns (uint256 _id) {
+   uint256 debtAuctionBidSize = _params.debtAuctionBidSize;
+   uint256 debtAuctionBidSize = _params.debtAuctionBidSize;

-   if (_params.debtAuctionBidSize == 0) revert AccEng_DebtAuctionDisabled();
+   if (debtAuctionBidSize == 0) revert AccEng_DebtAuctionDisabled();

    uint256 _coinBalance = safeEngine.coinBalance(address(this));
    uint256 _debtBalance = safeEngine.debtBalance(address(this));

-   if (_params.debtAuctionBidSize > _unqueuedUnauctionedDebt(_debtBalance)) revert AccEng_InsufficientDebt();
+   if (debtAuctionBidSize > _unqueuedUnauctionedDebt(_debtBalance)) revert AccEng_InsufficientDebt();

    (_coinBalance, _debtBalance) = _settleDebt(_coinBalance, _debtBalance, _coinBalance);
-   totalOnAuctionDebt += _params.debtAuctionBidSize;
+   totalOnAuctionDebt += debtAuctionBidSize;

    _id = debtAuctionHouse.startAuction({
      _incomeReceiver: address(this),
-     _amountToSell: _params.debtAuctionMintedTokens,
-     _initialBid: _params.debtAuctionBidSize // @audit
+     _amountToSell: debtAuctionMintedTokens,
+     _initialBid: debtAuctionBidSize 
    });

-   emit AuctionDebt(_id, _params.debtAuctionMintedTokens, _params.debtAuctionBidSize); // @audit
+   emit AuctionDebt(_id, debtAuctionMintedTokens, debtAuctionBidSize);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L192
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L188-L189
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L181
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L176

State struct Variable _params could first cached to memory and its property could used further.

function auctionSurplus() external returns (uint256 _id) {
+   AccountingEngineParams memory params = _params;

-   if(_params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit(); 
-   if (_params.surplusAmount == 0) revert AccEng_NullAmount();
+   if(params.surplusTransferPercentage > WAD) revert AccEng_surplusTransferPercentOverLimit(); 
+   if (params.surplusAmount == 0) revert AccEng_NullAmount();
    if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();
-   if (block.timestamp < lastSurplusTime + _params.surplusDelay) revert AccEng_SurplusCooldown();
+   if (block.timestamp < lastSurplusTime + params.surplusDelay) revert AccEng_SurplusCooldown();

    uint256 _coinBalance = safeEngine.coinBalance(address(this));
    uint256 _debtBalance = safeEngine.debtBalance(address(this));
    (_coinBalance, _debtBalance) = _settleDebt(_coinBalance, _debtBalance, _unqueuedUnauctionedDebt(_debtBalance));

-   if (_coinBalance < _debtBalance + _params.surplusAmount + _params.surplusBuffer) {
+   if (_coinBalance < _debtBalance + _params.surplusAmount + params.surplusBuffer) {
      revert AccEng_InsufficientSurplus();
    }

    // auction surplus percentage
    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
      _id = surplusAuctionHouse.startAuction({
-       _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
+       _amountToSell: params.surplusAmount.wmul(ONE_HUNDRED_WAD - params.surplusTransferPercentage),
        _initialBid: 0
      });

      lastSurplusTime = block.timestamp;
-     emit AuctionSurplus(_id, 0, _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage));
+     emit AuctionSurplus(_id, 0, params.surplusAmount.wmul(ONE_HUNDRED_WAD - params.surplusTransferPercentage));
    }

    // transfer surplus percentage
-   if (_params.surplusTransferPercentage > 0) {
+   if (params.surplusTransferPercentage > 0) {
      if (extraSurplusReceiver == address(0)) revert AccEng_NullSurplusReceiver();

      safeEngine.transferInternalCoins({
        _source: address(this),
        _destination: extraSurplusReceiver,
-       _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage)
+       _rad: _params.surplusAmount.wmul(params.surplusTransferPercentage)
      });

      lastSurplusTime = block.timestamp;
-     emit TransferSurplus(extraSurplusReceiver, _params.surplusAmount.wmul(_params.surplusTransferPercentage));
+     emit TransferSurplus(extraSurplusReceiver, params.surplusAmount.wmul(params.surplusTransferPercentage));
    }
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L199
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L200
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L202
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L208
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L220
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L230
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L234

_proxyRegistry[_proxy] could stored in memory variable in mint() function

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

+   require(proxy != address(0), 'V721: non-native proxy');
+   address _user = proxy;
    _safeMint(_user, _safeId);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L96-L97

[Gas-03] Use switch case instead of if-else condition which is more gas efficient

function _modifyParameters(bytes32 _param, bytes memory _data) internal override {
    uint256 _uint256 = _data.toUint256();
    address _address = _data.toAddress();

    // params
    if (_param == 'surplusTransferPercentage') _params.surplusTransferPercentage = _uint256; // @audit use switch case
    else if (_param == 'surplusDelay') _params.surplusDelay = _uint256;
    else if (_param == 'popDebtDelay') _params.popDebtDelay = _uint256;
    else if (_param == 'disableCooldown') _params.disableCooldown = _uint256;
    else if (_param == 'surplusAmount') _params.surplusAmount = _uint256;
    else if (_param == 'debtAuctionBidSize') _params.debtAuctionBidSize = _uint256;
    else if (_param == 'debtAuctionMintedTokens') _params.debtAuctionMintedTokens = _uint256;
    else if (_param == 'surplusBuffer') _params.surplusBuffer = _uint256;
    // registry
    else if (_param == 'surplusAuctionHouse') _setSurplusAuctionHouse(_address);
    else if (_param == 'debtAuctionHouse') debtAuctionHouse = IDebtAuctionHouse(_address);
    else if (_param == 'postSettlementSurplusDrain') postSettlementSurplusDrain = _address;
    else if (_param == 'extraSurplusReceiver') extraSurplusReceiver = _address;
    else revert UnrecognizedParam();
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L288-L301

[Gas-04] Store result of arithmatic operation in memory variable and then use further it instaed of calculating same expression again and again

  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
+   uint256 res = _deltaWad * RAY;
-   if (_coinAmount < _deltaWad * RAY) { // @audit cache _deltaWad * RAY
+   if (_coinAmount < res) {
      // 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();
+     _deltaDebt = ((res - _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;
    }
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L41
https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L43

  function _collectAndExitCoins(address _manager, address _coinJoin, uint256 _safeId, uint256 _deltaWad) internal {
    // Moves the COIN amount to proxy's address
+   uint256 res = _deltaWad * RAY;

-   _transferInternalCoins(_manager, _safeId, address(this), _deltaWad * RAY); 
+   _transferInternalCoins(_manager, _safeId, address(this), res);
    // Exits the COIN amount to the user's address
-   _exitSystemCoins(_coinJoin, _deltaWad * RAY);
+   _exitSystemCoins(_coinJoin, res);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L203-L205

[Gas-05] Instead of using var + 1, use var++ which is more gas efficient

-    _deltaWad = _deltaWad * RAY < _rad ? _deltaWad + 1 : _deltaWad;
+    _deltaWad = _deltaWad * RAY < _rad ? _deltaWad++ : _deltaWad;

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L87

[Gas-06] No need to cache function call result when it only used once

 internal {
-   address _safeEngine = ODSafeManager(_manager).safeEngine(); // @audit no need to cache
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);
    ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);

    // Takes token amount from user's wallet and joins into the safeEngine
    _joinCollateral(_collateralJoin, _safeInfo.safeHandler, _collateralAmount);

    // Locks token amount into the SAFE and generates debt
    _modifySAFECollateralization(
      _manager,
      _safeId,
      _collateralAmount.toInt(),
-     _getGeneratedDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
+     _getGeneratedDeltaDebt(ODSafeManager(_manager).safeEngine(), _safeInfo.collateralType, _safeInfo.safeHandler, _deltaWad)
    );

    // Exits and transfers COIN amount to the user's address
    _collectAndExitCoins(_manager, _coinJoin, _safeId, _deltaWad);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L179

  function repayDebtAndFreeTokenCollateral(
    address _manager,
    address _taxCollector,
    address _collateralJoin,
    address _coinJoin,
    uint256 _safeId,
    uint256 _collateralWad,
    uint256 _debtWad
  ) external delegateCall {
-   address _safeEngine = ODSafeManager(_manager).safeEngine(); // @audit no chahe
    ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);
    ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);

    // Joins COIN amount into the safeEngine
    _joinSystemCoins(_coinJoin, _safeInfo.safeHandler, _debtWad);

    // Paybacks debt to the SAFE and unlocks token amount from it
    _modifySAFECollateralization(
      _manager,
      _safeId,
      -_collateralWad.toInt(),
-     _getRepaidDeltaDebt(_safeEngine, _safeInfo.collateralType, _safeInfo.safeHandler)
+     _getRepaidDeltaDebt(ODSafeManager(_manager).safeEngine(), _safeInfo.collateralType, _safeInfo.safeHandler)
    );

    // Transfers token amount to the user's address
    _collectAndExitCollateral(_manager, _collateralJoin, _safeId, _collateralWad);
  }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/actions/BasicActions.sol#L354

[Gas-07] _safeData[_safes[_i]] could be cached inside for loop

function getSafesData(address _usr)
    external
    view
    returns (uint256[] memory _safes, address[] memory _safeHandlers, bytes32[] memory _cTypes)
  {
    _safes = _usrSafes[_usr].values();
    _safeHandlers = new address[](_safes.length);
    _cTypes = new bytes32[](_safes.length);
    for (uint256 _i; _i < _safes.length; _i++) {
+     SAFEData memory data = _safeData[_safes[_i]]; 
-     _safeHandlers[_i] = _safeData[_safes[_i]].safeHandler; 
-     _cTypes[_i] = _safeData[_safes[_i]].collateralType;

+     _safeHandlers[_i] = data.safeHandler; 
+     _cTypes[_i] = data.collateralType;
    }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L92-L93

[Gas-08] Uncheck those operations which will not overflow/underflow

 function openSAFE(bytes32 _cType, address _usr) external returns (uint256 _id) {
    if (_usr == address(0)) revert ZeroAddress();

-   ++_safeId;
+   unchecked{ ++_safeId; }

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/ODSafeManager.sol#L121