Lines of code
<https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/AccountingEngine.sol#L230&gt;
<https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/libraries/Math.sol#L104&gt;

Vulnerability details

Impact

The impact of this vulnerability is significant as it leads to a scaling problem in surplus auctions conducted by the AccountingEngine. The vulnerability arises from the way the _amountToSell in surplusAuctionHouse.startAuction call and _rad in safeEngine.transferInternalCoins call are calculated, causing an unintended scaling factor. The key points of impact are as follows:

Loss of Internal Coins: Take _amountToSell for example (the same situation can apply to _rad), at line 215, the wmul product of _params.surplusAmount and (ONE_HUNDRED_WAD - _params.surplusTransferPercentage) is denominated by WAD, which is approximately 1%. In contrast, the value of (ONE_HUNDRED_WAD - _params.surplusTransferPercentage) can be as high as 100 WAD. This difference in scaling results in a severe overestimation of the amount to be sold in surplus auctions, which leads to the draining of internal coins.

// File: src/contracts/AccountingEngine.sol
198:  function auctionSurplus() external returns (uint256 _id) {
    ...
213:    if (_params.surplusTransferPercentage < ONE_HUNDRED_WAD) {
214:      _id = surplusAuctionHouse.startAuction({
215:        _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage), // <= FOUND
216:        _initialBid: 0
217:      });
218:
219:      lastSurplusTime = block.timestamp;
      ...
236:  }

Proof of Concept

--- a/test/unit/AccountingEngine.t.sol
+++ b/test/unit/AccountingEngine.t.sol
@@ -762,6 +762,7 @@ contract Unit_AccountingEngine_AuctionDebt is Base {
 
 contract Unit_AccountingEngine_AuctionSurplus is Base {
   event AuctionSurplus(uint256 indexed _id, uint256 _initialBid, uint256 _surplusAuctioned);
+  event TransferSurplus(address indexed _extraSurplusReceiver, uint256 _surplusTransferred);
 
   struct AuctionSurplusScenario {
     uint256 surplusAmount;
@@ -857,6 +858,36 @@ contract Unit_AccountingEngine_AuctionSurplus is Base {
     accountingEngine.auctionSurplus();
   }
 
+  function test_WrongAuctionAndTransferAmountAccounting() public {
+    AuctionSurplusScenario memory _scenario = AuctionSurplusScenario({
+      surplusAmount: 1e18,
+      surplusBuffer: 1e18,
+      coinBalance: 2e18
+    });
+    _mockSurplusAmount(_scenario.surplusAmount);
+    _mockSurplusBuffer(_scenario.surplusBuffer);
+    _mockCoinAndDebtBalance(_scenario.coinBalance, 0);
+    // mock mockSurplusAuctionHouse.startAuction disregarding function input arguments
+    vm.mockCall(
+      address(mockSurplusAuctionHouse),
+      abi.encodeWithSelector(ICommonSurplusAuctionHouse.startAuction.selector),
+      abi.encode(1)
+    );
+    _mockExtraSurplusReceiver(address(999));
+
+    _mocksurplusTransferPercentage(1e18); // WAD ~ 1%
+    vm.expectEmit();
+    // Error 1: _surplusAuctioned not accounted for 99%, but scalled up 99 / 0.99 = 100 times
+    emit AuctionSurplus(1, 0, _scenario.surplusAmount * 99);
+    vm.expectEmit();
+    // Error 2: _surplusTransferred not accounted for 1%, but scalled up 1 / 0.01 = 100 times
+    emit TransferSurplus(address(999), _scenario.surplusAmount);
+    accountingEngine.auctionSurplus();
+
+    // @audit: Passing this test will prove that a total amount of _scenario.surplusAmount * 100 were transfered out of accountingEngine. 
+    // This would have a negative impact on the protocol accounting
+  }
+
   function test_Revert_surplusTransferPercentageIs1() public {
     _mocksurplusTransferPercentage(1);

Results:

> Running 1 test for test/unit/AccountingEngine.t.sol:Unit_AccountingEngine_AuctionSurplus
[PASS] test_WrongAuctionAndTransferAmountAccounting() (gas: 340592)
diff --git a/test/unit/AccountingEngine.t.sol b/test/unit/AccountingEngine.t.sol
index 472b5c3…3a19a0f 100644

The passed test proves that a total amount of _scenario.surplusAmount * 100 were transfered out of accountingEngine.

Tools Used

Vscode + Foundry test

Recommended Mitigation Steps

- _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage),
+ _amountToSell: _params.surplusAmount.wmul(ONE_HUNDRED_WAD - _params.surplusTransferPercentage) / 100,


- _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage)
+ _rad: _params.surplusAmount.wmul(_params.surplusTransferPercentage) / 100

Assessed type

Math

---

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

All reactions