Mismatch between the SAFE generated debt and the amount of the system tokens minted for the user

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

pi0neerpat (sponsor) confirmed

[c4-judge]

MiloTruck marked the issue as satisfactory

[c4-judge]

MiloTruck marked the issue as selected for report

[MiloTruck]

In _generateDebt(), _collectAndExitCoins() is called to transfer system coins equivalent to the requested amount of collateral instead of the actual amount used for debt. This will cause the function to revert should _deltaWad be higher than the amount of collateral the user has in his safe to generate debt.

Since this is a case of the function not working as intended, I believe medium severity is appropriate.