hats-finance / Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd

GNU Affero General Public License v3.0
2 stars 0 forks source link

Adversary can block any `exit` due to `preCheck` reached `cap` by using flash-loan #27

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xa152e0fc679f9840949b13a8713252bd1f4f032c33244d6bff807347e639bce3 Severity: medium

Description: Description

There is a maximum dailycap implemented on circuitBreaker contract to prevent any abnormal ovUSDC exits by users.

The preCheck will increment current bucketIndex amount, beside checking if the sum of rolling period buckets is still under the cap.


File: OrigamiCircuitBreakerAllUsersPerPeriod.sol
098:     function preCheck(address /*onBehalfOf*/, uint256 amount) external override onlyProxy {
...
118:         uint256 _newUtilisation = _currentUtilisation(_nBuckets) + amount;
119:         if (_newUtilisation > cap) revert CapBreached(_newUtilisation, cap);
120: 
121:         // Unchecked is safe since we know the total new utilisation is under the cap.
122:         unchecked {
123:             // slither-disable-next-line weak-prng
124:             buckets[_nextBucketIndex % _nBuckets] += amount;
125:         }
126:     }

The issue here is, attacker can flash-loan in order to fill-up the rolling period until it reached its cap.

By using flash-loaned USDC, then investWithToken oUSDC and exitToToken in a single transaction. This flash loan can trigger preCheck and fill up the cap easily.

File: OrigamiLendingSupplyManager.sol
146:     function exitToToken(
147:         address account,
148:         IOrigamiInvestment.ExitQuoteData calldata quoteData,
149:         address recipient
150:     ) external override onlyOToken returns (
151:         uint256 toTokenAmount,
152:         uint256 toBurnAmount
153:     ) {
...
157:         // Ensure that this exit doesn't break the circuit breaker limits for oToken
158:         circuitBreakerProxy.preCheck(
159:             address(oToken),
160:             account,
161:             quoteData.investmentTokenAmount
162:         );

Attack Scenario

  1. User get a flash-loaned asset USDC
  2. User buys oUSDC (investWithToken) with some amount of USDC from the flash-loan OrigamiInvestmentVault::investWithToken() -> OrigamiOToken::investWithToken() -> OrigamiLendingSupplyManager::investWithToken() -> OrigamiLendingClerk::deposit, there is no fee deducted here, thus USDC -> oUSDC will be 1:1, and get shares ovUSDC
  3. User exit to USDC again, by calling OrigamiLendingSupplyManager::exitToToken() and passing all shares, to circuit breaker preCheck (and save it to current index bucket), then fill it near the cap
  4. in the rolling period, the cap is now filled, any exits (or rebalance) will be reverted.

This issue can cause temporary DoS, preventing legitimate user exit ovUSDC normally due to filled cap.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

Recommendation

Consider implementing a preventive measure to disallow both invest and exit in the same block. This would introduce a delay or separation between these operations, reducing the risk of the flash-loan attack scenario.

chainNue commented 6 months ago

To make it clear, the flow would be:

  1. Flash-loan USDC
  2. Check circuit breaker's cap and currentUtilisation() (for oUSDC)
  3. Get amount to fill up the gap between cap and currentUtilisation
  4. investWithToken, from USDC -> oUSDC, fromTokenAmount = gap amount
  5. exitToToken, from oUSDC -> USDC, investmentTokenAmount = gap amount
  6. this exitToToken in manager will trigger preCheck, updating bucket to cap, fill up utilisation.

Using these steps which is done in one transaction can freeze user exit due to daily cap reached

frontier159 commented 6 months ago

Thanks for the finding @chainNue

While there's no economic benefit, someone could do this as a DoS.

We'll consider if we remediate with:

chainNue commented 6 months ago

Thanks @frontier159 for accepting this issue (and my other issue as well),

Indeed, there is no economic benefit, a short-term freezing of user funds is a medium one. If there is no mechanism to mitigate this issue, someone can easily block exit by filling up the cap, thus valid user hardly exit their asset, unless protocol intervene by increasing the cap, but that not preventing user doing the same attack again (or the cap lose its meaning)

Agree with the solution you're going to take, a cooldown and an exit fee I think is enough to mitigate this issue.

frontier159 commented 6 months ago

Agree with the solution you're going to take, a cooldown and an exit fee I think is enough to mitigate this issue.

To be clear i think either of those is enough, don't need both

chainNue commented 6 months ago

yes, you are right, it's an OR not an AND

JacoboLansac commented 6 months ago

Another solution is to disable exits on the same block number as investments. Kind of a super short cooldown, only affecting invest-and-exit in the same transaction (flash loans).