code-423n4 / 2023-10-opendollar-findings

10 stars 7 forks source link

AccountingEngine can only transfer a maximum of 1% of the surplus to the designated extraSurplusReceiver #384

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The impact of this vulnerability is related to the validation of the surplus transfer percentage in the AccountingEngine contract. The vulnerability arises from an incorrect validation check that restricts the surplus transfer percentage to WAD ~ 1%.

Proof of Concept

The validation logic checks if the _params.surplusTransferPercentage exceeds WAD (1%), causing a revert if the condition is met. According to Dev's comment regarding AccEng_surplusTransferPercentOverLimit error, the limit of surplusTransferPercentage should be 100% which is represented by ONE_HUNDRED_WAD.

// File: src/interfaces/IAccountingEngine.sol
88:  /// @notice Throws when trying to transfer post-settlement surplus before the disable cooldown has passed
89:  error AccEng_PostSettlementCooldown();
90:  /// @notice Throws when surplus surplusTransferPercentage is great than WAD (100%) // <= FOUND
91:  error AccEng_surplusTransferPercentOverLimit();
92:
93:  // --- Structs ---
94:
  ...
112:  }

However, the validation incorrectly reverts when the surplus transfer percentage exceeds ONLY 1WAD. This leads to the unintended and excessive restriction of the protocol's ability to transfer surplus. As a result, the protocol can only transfer a maximum of 1% of the surplus to the designated receiver, with the remaining 99% sent to auction. This unnecessary validation limits the protocol's control range and affects its flexibility

Tools Used

Manual Review

Recommended Mitigation Steps

Modify the validation check to compare the _params.surplusTransferPercentage against ONE_HUNDRED_WAD to ensure that the percentage does not exceed the upper limit of 100%. The validation logic should be updated as follows:

- if (_params.surplusTransferPercentage > WAD) {
+ if (_params.surplusTransferPercentage > ONE_HUNDRED_WAD) {
     revert AccEng_surplusTransferPercentOverLimit();
 }

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #26

c4-judge commented 1 year ago

MiloTruck changed the severity to 3 (High Risk)

c4-judge commented 1 year ago

MiloTruck marked the issue as satisfactory

MiloTruck commented 1 year ago

The warden has found the other half of the bug in #368, so I'm not giving this partial credit.