Idle Finance

Quantstamp23 Apr 2021Files named in the report

Audit

Quantstamp reviewed Idle Finance on 23 Apr 2021: 39 findings, 0 critical; 22 marked fixed, 14 acknowledged, 3 open.

No open critical findings

Open the report Open Idle Finance’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings39

Statuses are the security firm’s own, from its report.

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	0	0
Medium	1	0	3
Low	2	14	19

Medium`redeemIdleToken()` may fail if `fee` is reset to zeroFixed

Assume that: A1: `userNoFeeQty[msg.sender]` can only accumulated when `fee` is set to `0` (according to the `_updateAvgPrice()` function). A2: the price of idleToken is `5` and does not change a lot (this happens when the `balanceUnderlying` is large). Consider the following scenario for some `user1`: 1. `user1` deposits `100` underlying token when `fee` is set to `0`. The `user1` will obtain `100/5 = 20` idleToken, and we noted that `userNoFeeQty[user1]` equals to `20` 2. Then the idleFinance team decides to change the `fee` from `0` to `1000`. 3. When the `user1` later deposit again, with another `100` underlaying token, the `user1` will obtain `100/5 = 20` idleToken again. In addition to the formerly obtained `20` idleToken, now the `user1` has `20 + 20 = 40` idleTokens on hand. However, since `fee != 0` now, the `userNoFeeQty[user1]` will remains equal to `20` instead of equal to `20 + 20 = 40`. 4. Then the idleFinance team decides to change the `fee` from `1000` to `0` again. 5. Finally, when `user1` decides to redeem idleTokens through function `redeemIdleToken()` by passing the parameter `_amount = 40`, we have that the `_amount` is `40` but the `userNoFeeQty[user1]` is `20`. This will cause the revert of the function due to the statement: `userNoFeeQty[msg.sender] = userNoFeeQty[msg.sender].sub(_amount);`.

MediumCentralization of PowerFixed

Smart contracts will often have `owner` variables to designate the person with special privileges to make modifications to the smart contract. In several contracts, the associated tokens may be changed by the owner. If the balances of the contracts are non-zero, users may not be able to retrieve funds or interact with the contract in a proper manner. In particular: * In `IdleFulcrum` and `IdleCompound`, tokens may be updated by `setToken()` and `setUnderlying()`. * In `IdleRebalancer.sol`, `setIdleToken()`, `setCToken()`, `setIToken()`, `setCTokenWrapper()`, and `setITokenWrapper()` may update underlying addresses. * In `IdleTokenV3` and `IdleRebalancerV3.sol`, the owner may add new token wrappers arbitrarily (which may not correspond to actual lending protocols). Additionally, the owner may pause/unpause certain functionalities, such as rebalancing.

MediumUser may not be able to redeem Idle tokensFixed

If the `_tokenPrice()` is lower than the `userAvgPrices` for that user, then the `sub` method call on L911 in `_getFee` will throw an error and revert the transaction. Given that the `_getFee` function is only called in `redeemIdleToken` it will lead to users not being able to redeem Idle tokens as long as the current price is lower than the `userAvgPrices` for that user.

Low`newIdleToken()` may overwrite `underlyingToIdleTokenMap[_token]`Fixed

If `newIdleToken()` is called with an existing `_token` address, the `IdleToken` contract referenced in the `underlyingToIdleTokenMap` will be overwritten. It is not clear if this is intended functionality.

LowClone-and-OwnFixed

The clone-and-own approach involves copying and adjusting open source code at one's own discretion. From the development perspective, it is initially beneficial as it reduces the amount of effort. However, from the security perspective, it involves some risks as the code may not follow the best practices, may contain a security vulnerability, or may include intentionally or unintentionally modified upstream libraries. In `IdleMcdBridge.sol`, there are several libraries that could be imported: `IERC20`, `SafeMath`, `Context`, and `Address`.

LowFunction `rebalance()` may be blocked due to Fulcrum failureFixed

On `L508` of `IdleTokenV3.sol`, the modifier `whenITokenPriceHasNotDecreased` checks that function `_rebalance` can only be executed when the iToken price has not decreased. However, since `Fulcrum` could get hacked (or the price of collateral may drop), it might not always be true. When this happens, the system would not be able to rebalance/reallocate funds for a period of time.

LowGas constants may be affected by new EVM forksFixed

In `GST2Consumer.sol`, several constants are defined related to gas usage. Since op-code gas costs may be updated in new forks, this may cause unforeseen gas issues in future forks.

LowGas Usage / `for` Loop ConcernsFixed

Gas usage is a main concern for smart contract developers and users, since high gas costs may prevent users from wanting to use the smart contract. Even worse, some gas usage issues may prevent the contract from providing services entirely. For example, if a `for` loop requires too much gas to exit, then it may prevent the contract from functioning correctly entirely. It is best to break such loops into individual functions as possible. In particular, the rebalancing functions may require several loops in the bisection algorithm.

LowInconsistent array lengths breaks invariantsFixed

The length of the `allAvailableTokens` array and the `lastRebalancerAllocations` and `lastAllocations` arrays may diverge after calling `setAllAvailableTokensAndWrappers`, even if they were the same length after `manualInitialize`. This is because the allocations are not adjusted or checked to be of the same length with the `protocolTokens` or `wrappers` input arrays. This means that the owner can remove tokens from the `allAvailableTokens` array and the sum of all corresponding allocations would not be 100% after that call.

LowIncorrect average price computationFixed

The `userNoFeeQtyFrom` part of the `qty` input parameter of the`_updateUserFeeInfo` function is subtracted twice from `totBalance`: on deposits on L889 and L892. See the following code snippet: ``` 889: uint256 totBalance = balanceOf(usr).sub(userNoFeeQty[usr]); 890: // noFeeQty should not be counted here 891: // (avgPrice * oldBalance) + (currPrice * newQty)) / totBalance 892: userAvgPrices[usr] = userAvgPrices[usr].mul(totBalance.sub(qty)).add(price.mul(qty)).div(totBalance); ``` This happens because `userNoFeeQtyFrom` was already added to `userNoFeeQty[usr]`, which is first subtracted on L889. This leads to an incorrect `userAvgPrice` for that user. <br> Additionally, the `price` should not be multiplied by `qty` on L892, because on transfers, the amount that is actually transfered to `usr` is equal to `userNoFeeQtyFrom`.

Show 10 more

LowLength of input arrays can be differentFixed

There are multiple occurrences of this issue: 1. There is no check in place inside the `redeemAllNeeded` function inside `IdleTokenV3_1`, which checks if the length of the `tokenAddresses`, `amounts` and the `newAmounts` input arrays are equal. Since the for-loop inside this function goes up to `amounts.length` it would be problematic if the lengths of the other arrays would be different (shorter or longer). 1. There is no check in place inside the `_mintWithAmounts` function inside `IdleTokenV3_1`, which checks if the length of the `tokenAddresses` and the `protocolAmounts` input arrays are equal. Since the for-loop inside this function goes up to `protocolAmounts.length` it would be problematic if the lengths of the other array would be different (shorter or longer). 1. There is no check in place inside the `setAllAvailableTokensAndWrappers` function inside `IdleTokenV3_1`, which checks if the length of the `protocolTokens` and the `allAvailableTokens` arrays have the same length. This could lead to removing or adding tokens and/or changing the order of the tokens w.r.t. the `lastAllocations` array order.

LowLoss of precision due to truncationFixed

The computation of the average APR inside the `getAvgAPR()` function, is performed by normalizing (dividing by `total`) the APR for each token separately and adding the normalized values together. Due to the limited precision and truncation of the division operation, there might be a loss of precision in this computation. Similarly the division by `10**18` can be moved outside of the for-loop in the `_getCurrentPoolValue` function.

LowMissing `onlyIdle` modifier on `mint()` and `redeem()`Fixed

For the functions `IdleCompoundV2.mint()` and `IdleCompoundV2.redeem()`, there is no `onlyIdle` modifier, whereas the modifier exists in the corresponding functions in `IdleCompound.sol`, `IdleFulcrum.sol`, and `IdleFulcrumV2.sol`. This would allow funds stored in the `IdleCompoundV2` wrapper contract to be sent to an arbitrary address. Although the typical dApp workflow does not store funds directly in the wrapper contract (in favor of storing balances in `IdleToken`, users interacting directly with the `IdleCompoundV2` wrapper contract may mistakenly add funds to the contract directly. Adding the `onlyIdle` modifier to these functions would mitigate these incorrect interactions.

LowOutdated `govToken` could be used to influence the average APRFixed

The following condition in `_getAvgAPR`, on L358: `if (govTokens.length > 0 && currGov != address(0))` only checks if the length of `govTokens` is greater than zero. However, it does not check if the length of the `govTokens` is greater than `i` (the loop iterator) or if the `currGov` is in the `govTokens` array. Due to the way in which the `setGovTokens` function works, it may be the case that `currGov != address(0)` but `currGov` is not included in the `govTokens` array. This could have very severe consequences because any user is allowed to call `openRebalance`, which changes the allocations based on the results obtained from calling `_getAvgAPR`. The `_getAvgAPR` function would return the wrong results, because it would take into consideration removed `govTokens`.

LowPotential flash loans attack vectors to claim COMP tokensFixed

After discussion with the Idle team, it appears that there may exist attack vectors that claim COMP tokens using flash loans, if a rebalance or redeem has not been invoked in a long time. This attack could occur if mint and redeem are invoked with a large balance in the same transaction (via a flash loan).

LowThe `flashLoanFee` is not settableFixed

The `flashLoanFee` cannot be changed by a function call after the contract is deployed. The only way to change it is to upgrade/redeploy the contract.

LowUnchecked constructor and setter address argumentsFixed

* In `IdleRebalancerV3.sol`, on L28, the constructor arguments `_yxToken` and `_rebalancerManager` were not checked to be non-zero. * In `IdleTokenV3.sol`, the constructor and all setter functions should check that addresses are non-zero.

LowUnclear functionality in `_getFee`Fixed

* The functionality of `L907`: `userNoFeeQty[msg.sender] = noFees ? noFeeQty.sub(amount) : 0;`, is unclear. It seems that what we want to achieve here is more like `userNoFeeQty[msg.sender] = balanceOf(msg.sender).sub(_amount);` when `fee == 0` and `userNoFeeQty[msg.sender] = noFeeQty.sub(amount)` when `noFeeQty >= amount`.

LowUnclear update to `userAvgPrices` mappingFixed

In the function `_updateAvgPrice`, the mapping `userAvgPrices` is not updated if the `fee == 0`. It is not clear why the mapping is not updated in this case, but since this case is not covered, the user's average price may not be correct in all scenarios.

LowUndocumented magic constantsFixed

There are several defined constants in the code that were unclear, and would benefit from added inline documentation: * In `IdleAave.sol`, L161: the number 29; * In `IdleAave.sol`, the constant on L143 of `getApr()`: `100/10^9`; * In `GST2Consumer.sol`, all numerical constants on L15, 19-20; * In `IdleRebalancerV3.sol`, on L32, it is not immediately clear that the constant 100000 is 100%.

Show 2 more

LowUnlocked PragmaFixed

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.4.*`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked." The file `IdleMcdBridge.sol` has several instances of unlocked pragmas throughout.

LowUse of ABIEncoderV2 still experimentalFixed

Until solidity 0.6.0, the ABIEncoderV2 feature is still technically in experimental state. Although there are no known security risks associated with it, these features should be used judiciously.