Lines of code
<https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/actions/BasicActions.sol#L31-L47&gt;
<https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/SAFEEngine.sol#L161-L162&gt;

Vulnerability details

Impact

Users can manipulate coinBalance mapping in the SafeEngine by calling ODSafeManager::transferInternalCoins in order to improve their collateralization rate generate more and repay less debt.

Proof of Concept

When a user wants to be able to generate debt and receive COIN tokens, he will call the BasicAction::generateDebt which will calculate the delta by taking his COIN balance and the total debt of his SAFE, these are the protocol assumptions. If a malicious user wants to be able to generate more he is allowed in order to maintain his collateralization he will do the following:

1. He will open a new safe by calling ODSafeManager::openSafe and deploy new SafeHandler on himself.

2. Call allowSafe and add himself to the safeCan mapping of the new SafeHandler.

3. Then he will call ODSafeManager::transferInternalCoins with the following params:

_safe: his new safe

_dst: his old safe

_rad: amount which to be transferred between safes

That will increase the coinBalance[src]

src: od-contracts/src/proxies/ODSafeManager.sol
function transferInternalCoins(uint256 _safe, address _dst, uint256 _rad) external safeAllowed(_safe) {
    SAFEData memory _sData = _safeData[_safe];
    ISAFEEngine(safeEngine).transferInternalCoins(_sData.safeHandler, _dst, _rad);
    emit TransferInternalCoins(msg.sender, _safe, _dst, _rad);
  }


src: od-contracts/src/SAFEEngine.sol
function transferInternalCoins(
    address _source,
    address _destination,
    uint256 _rad
  ) external isSAFEAllowed(_source, msg.sender) {
    coinBalance[_source] -= _rad;
    coinBalance[_destination] += _rad;
    emit TransferInternalCoins(_source, _destination, _rad);
  }

4. Then he will call BasicAction::generateDebt and enter the internal function _collectAndExitCoins which has the following structure:

src: od-contracts/src/proxies/actions/BasicActions.sol
/**
   * @notice Generates debt
   * @dev    Modifies the SAFE collateralization ratio, increasing the debt and sends the COIN amount to the user's address
   */
  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)
    );

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


src: od-contracts/src/proxies/actions/BasicActions.sol
/**
   * @notice Gets delta debt generated for delta wad (always positive)
   * @dev    Total SAFE debt minus available safeHandler COIN balance
   */
  function _getGeneratedDeltaDebt(
    address _safeEngine,
    bytes32 _cType,
    address _safeHandler,
    uint256 _deltaWad
  ) internal view returns (int256 _deltaDebt) {
    uint256 _rate = ISAFEEngine(_safeEngine).cData(_cType).accumulatedRate; //1
    //Borrowed tokens
//@audit there will be a discrepancy because coinBalance was directly modified from the odProxy owner
    uint256 _coinAmount = ISAFEEngine(_safeEngine).coinBalance(_safeHandler); // e18

    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
      //@audit needed deltaDebt will be much lower and show less debt than usual for this user
			_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;
    }
  }

5. Flow will enter the SafeEngine contract which has nothing to do with the user manipulated params:

src: od-contracts/src/contracts/SAFEEngine.sol
/// @inheritdoc ISAFEEngine
  function modifySAFECollateralization(
    bytes32 _cType,
    address _safe,
    address _collateralSource,
    address _debtDestination,
    int256 _deltaCollateral, //positive
    int256 _deltaDebt
  ) external whenEnabled {
    SAFEEngineCollateralData storage __cData = _cData[_cType];
    // collateral type has been initialised
    if (__cData.accumulatedRate == 0) revert SAFEEng_CollateralTypeNotInitialized();

    _modifyCollateralBalance(_cType, _collateralSource, -_deltaCollateral); 
    _emitTransferCollateral(_cType, address(0), _safe, _deltaCollateral);
    _modifySAFECollateralization(_cType, _safe, _deltaCollateral, _deltaDebt);
    __cData.debtAmount = __cData.debtAmount.add(_deltaDebt);
    __cData.lockedAmount = __cData.lockedAmount.add(_deltaCollateral);
		
//@audit this variable will be overinflated and will lead to discrepancy in the globalDebt variable calculated inside _modifyInternalCoins
    int256 _deltaAdjustedDebt = __cData.accumulatedRate.mul(_deltaDebt);
    _modifyInternalCoins(_debtDestination, _deltaAdjustedDebt);

    // --- Safety checks ---
    {
      SAFEEngineCollateralParams memory __cParams = _cParams[_cType];
      SAFE memory _safeData = _safes[_cType][_safe];
      uint256 _totalDebtIssued = __cData.accumulatedRate * _safeData.generatedDebt;

      // either debt is increased (generated) and debt ceilings are not exceeded, or debt destination consents
      if (_deltaDebt > 0) {
        if (globalDebt > _params.globalDebtCeiling) revert SAFEEng_GlobalDebtCeilingHit();
        if (__cData.debtAmount * __cData.accumulatedRate > __cParams.debtCeiling) {
          revert SAFEEng_CollateralDebtCeilingHit();
        }
        if (_safeData.generatedDebt > _params.safeDebtCeiling) revert SAFEEng_SAFEDebtCeilingHit();
      } else {
        if (!canModifySAFE(_debtDestination, msg.sender)) revert SAFEEng_NotDebtDstAllowed();
      }

      // either safe is less risky, or it is still safe and the owner consents
      if (_deltaDebt > 0 || _deltaCollateral < 0) {
        if (_totalDebtIssued > _safeData.lockedCollateral * __cData.safetyPrice) revert SAFEEng_SAFENotSafe();
        if (!canModifySAFE(_safe, msg.sender)) revert SAFEEng_NotSAFEAllowed();
      }

      // either collateral is decreased (returned), or collateral source consents
      if (_deltaCollateral > 0) {
        if (!canModifySAFE(_collateralSource, msg.sender)) revert SAFEEng_NotCollateralSrcAllowed();
      }

      // either safe has no debt, or a non-dusty amount
      if (_safeData.generatedDebt != 0 && _totalDebtIssued < __cParams.debtFloor) revert SAFEEng_DustySAFE();
    }

    emit ModifySAFECollateralization(_cType, _safe, _collateralSource, _debtDestination, _deltaCollateral, _deltaDebt);
  }

6. Then he will transfer back the COIN tokens by calling ODSafeManager::transferInternalCoins transferring back the tokens that he had initially.

*in order to execute the vulnerability successfully user must have more COINs in the source SafeHandler that he will use to gain the advantage in order to pass all the checks in BasicAction::generateDebt function

Tools Used

Manual Review

Recommended Mitigation Steps

Consider disallowing functions from ODSafeManger and modify mappings of the SafeEngine freely.

Assessed type

Invalid Validation

---

The text was updated successfully, but these errors were encountered:

All reactions