repayAllDebt joins system tokens to the proxy not to the Safe contract

[c4-submissions]

Lines of code

https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/actions/BasicActions.sol#L282-L310 https://github.com/open-dollar/od-contracts/blob/67e5917e7dc0c16324aff3fde0298cd218a15152/src/contracts/proxies/actions/BasicActions.sol#L374-L400

Vulnerability details

Impact

repayAllDebt now calls _joinSystemCoins with dest = address(this), which in case a proxy makes a delegated call to this function will cause the user's tokens to be transferred to its proxy, then in CoinJoin.join() it will transfer internal coins from CoinJoin to the proxy and will burn system tokens from the proxy, which will cause all previously transferred system tokens to be burned, which will make calling the whole function illogical because the token transfer will get messed up which will also mess up the user debt.

Proof of Concept

As you may see repayAllDebt logic is also present in repayAllDebtAndFreeTokenCollateral but both functions do not make the same in terms of repaying all available debt. repayAllDebt join the tokens to address(this) [user’s proxy], when repayAllDebtAndFreeTokenCollateral join the tokens to _safeInfo.safeHandler, as it right to be.

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

  ISAFEEngine.SAFE memory _safeData = ISAFEEngine(_safeEngine).safes(_safeInfo.collateralType, _safeInfo.safeHandler);

  // Joins COIN amount into the safeEngine
  _joinSystemCoins(
    _coinJoin,
    address(this),
    _getRepaidDebt(_safeEngine, address(this), _safeInfo.collateralType, _safeInfo.safeHandler)
  );

  // Paybacks debt to the SAFE (allowed because reducing debt of the SAFE)
  ISAFEEngine(_safeEngine).modifySAFECollateralization({
    _cType: _safeInfo.collateralType,
    _safe: _safeInfo.safeHandler,
    _collateralSource: address(this),
    _debtDestination: address(this),
    _deltaCollateral: 0,
    _deltaDebt: -int256(_safeData.generatedDebt)
  });
}

function repayAllDebtAndFreeTokenCollateral(
  address _manager,
  address _taxCollector,
  address _collateralJoin,
  address _coinJoin,
  uint256 _safeId,
  uint256 _collateralWad
) external delegateCall {
  address _safeEngine = ODSafeManager(_manager).safeEngine();
  ODSafeManager.SAFEData memory _safeInfo = ODSafeManager(_manager).safeData(_safeId);
  ITaxCollector(_taxCollector).taxSingle(_safeInfo.collateralType);

  ISAFEEngine.SAFE memory _safeData = ISAFEEngine(_safeEngine).safes(_safeInfo.collateralType, _safeInfo.safeHandler);

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

  // Paybacks debt to the SAFE and unlocks token amount from it
  _modifySAFECollateralization(_manager, _safeId, -_collateralWad.toInt(), -_safeData.generatedDebt.toInt());

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

Tools Used

Manual Review

Recommended Mitigation Steps

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

  ISAFEEngine.SAFE memory _safeData = ISAFEEngine(_safeEngine).safes(_safeInfo.collateralType, _safeInfo.safeHandler);

  // Joins COIN amount into the safeEngine
  _joinSystemCoins(
    _coinJoin,
-   address(this),
-   _getRepaidDebt(_safeEngine, address(this), _safeInfo.collateralType, _safeInfo.safeHandler)
+   _safeInfo.safeHandler,
+   _getRepaidDebt(_safeEngine, _safeInfo.safeHandler, _safeInfo.collateralType, _safeInfo.safeHandler)
  );

  // Paybacks debt to the SAFE (allowed because reducing debt of the SAFE)
  ISAFEEngine(_safeEngine).modifySAFECollateralization({
    _cType: _safeInfo.collateralType,
    _safe: _safeInfo.safeHandler,
    _collateralSource: address(this),
    _debtDestination: address(this),
    _deltaCollateral: 0,
    _deltaDebt: -int256(_safeData.generatedDebt)
  });
}

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Insufficient proof.

[c4-judge]

MiloTruck marked the issue as unsatisfactory: Invalid

[MiloTruck]

_joinSystemCoins() in repayAllDebt() is meant to add to the proxy's internal coin balance since the proxy calls modifySAFECollateralization() with itself as the _collateralSource and _debtDestination afterwards.