`OrigamiLendingSupplyManager::maxExit()` does not account for circuit breaker cap when returning the max exit amount
Opened this issue · 1 comments
Github username: @JacoboLansac
Twitter username: jacolansac
Submission hash (on-chain): 0x29ced213355051d3f9da98b6d376d832fd19404c0c725a946e6ae58d8abf74a9
Severity: low
Description:
In the oUSDC system, the OrigamiLendingSupplyManager
contract handles the exit of oUSDC
(in exchange for USDC
) with the following function. This function invokes a call to the circuit breaker that caps the oUSDC
that can be burned within a 24 h period. I understand that this is to limit the amount of USDC
that can be withdrawn from the oUSDC
system per 24h.
/**
* @notice Sell oToken to receive `asset`.
* param account The account to exit on behalf of
* @param quoteData The quote data received from exitQuote()
* @param recipient The receiving address of the `asset`
* @return toTokenAmount The number of `asset` tokens received upon selling the oToken.
* @return toBurnAmount The number of oToken to be burnt after exiting this position
*/
function exitToToken(
address account,
IOrigamiInvestment.ExitQuoteData calldata quoteData,
address recipient
) external override onlyOToken returns (
uint256 toTokenAmount,
uint256 toBurnAmount
) {
if (_paused.exitsPaused) revert CommonEventsAndErrors.IsPaused();
if (quoteData.toToken != address(asset)) revert CommonEventsAndErrors.InvalidToken(quoteData.toToken);
@> // Ensure that this exit doesn't break the circuit breaker limits for oToken
@> circuitBreakerProxy.preCheck(
address(oToken),
account,
quoteData.investmentTokenAmount
);
// No exit fees, receive `asset` 1:1
// This scaleDown intentionally rounds down (so it's not in the user's benefit)
toBurnAmount = quoteData.investmentTokenAmount;
toTokenAmount = quoteData.investmentTokenAmount.scaleDown(_assetScalar, OrigamiMath.Rounding.ROUND_DOWN);
if (toTokenAmount != 0) {
lendingClerk.withdraw(toTokenAmount, recipient);
}
}
On the other hand, the same contract implements the maxExit()
function which should return the maximum amount of USDC
that a user should expect to be able to withdraw at a given moment. At the moment, it returns whatever the lendingClerk
deems as available to withdraw, but it does not compare it to the circuit breaker for oUSDC
tokens.
/**
* @notice The maximum amount of fromToken's that can be deposited
* taking any other underlying protocol constraints into consideration
* For lending supply oToken's, this is bounded by the currently available
* amount of the supply asset that can be withdrawn
*/
function maxExit(address toToken) external override view returns (uint256 amount) {
if (toToken == address(asset)) {
// No exit fees, receive `asset` 1:1.
// Convert from the underlying asset to the oToken decimals
@> amount = lendingClerk.totalAvailableToWithdraw().scaleUp(_assetScalar);
}
}
This function is invoked by the OrigamiOToken
contract:
function maxExit(address toToken) external override view returns (uint256 amount) {
@> amount = manager.maxExit(toToken);
}
Impact: low
The OrigamiLendingSupplyManager::maxExit()
function will give higher values than the ones allowed by the circuit breaker most of the time. If users read this function to see how much they can withdraw, it will mislead them, showing them higher values than what the circuit breaker will allow. This may lead to reverted transactions because of users attempting more than what the circuit breaker allows.
NOTE: ``
Recommendation
Check the circuit breaker limit and compare it to the available to withdraw amount from the lending clerk:
function maxExit(address toToken) external override view returns (uint256 amount) {
if (toToken == address(asset)) {
// No exit fees, receive `asset` 1:1.
// Convert from the underlying asset to the oToken decimals
- amount = lendingClerk.totalAvailableToWithdraw().scaleUp(_assetScalar);
+ // the circuit breaker is in oTokens, so 18 decimals
+ uint256 circuitBreakerRemainder = circuitBreakerProxy.cap() - circuitBreakerProxy.currentUtilisation();
+ uint256 amountFromLendingClerk = lendingClerk.totalAvailableToWithdraw().scaleUp(_assetScalar);
+ amount = (circuitBreakerRemainder < amountFromLendingClerk) ? circuitBreakerRemainder : amountFromLendingClerk;
}
}
PoC
- Let's assume there is 1,000,000 USDC deposited in the oUSDC system, and a 24h cap of 10,000 USDC for withdraws.
- Let's assume Alice and Bob had deposited 10,000 USDC each.
- Alice reads maxExit(), which returns 1000,000, and withdraws 8000. OK.
- On the same day, Bob reads maxExit(), which returns 1,000,000-8000=992,000. Attempts to withdraw 5000, but the circuit breaker only allows 2000, so the transaction reverts
- Bob is angry because the contracts said he could have withdrawn 992,000
We don't want to make Bob angry - thanks. Agree a low since it's just a UX issue