Ichi

Quantstamp30 Apr 2021Files named in the report

Audit

Quantstamp reviewed ICHI on 30 Apr 2021: 29 findings, 0 critical; 21 marked fixed, 8 acknowledged.

No open critical findings

Open the report Open ICHI’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings29

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

Severity	Acknowledged	Fixed
Critical	0	0
High	0	4
Medium	1	3
Low	7	14

HighMinting fee can be avoided if insufficient member tokens are providedFixed

The function `mint` adds a fee to the collateral USD value on L116: `collateralUSDValue = collateralUSDValue.add(collateralUSDValue.mul(mintingFee).div(PRECISION));`. If the member token allowance is too low, the `collateralUSDValue` is recomputed in L125-130 based on the amount of member tokens that the user can provide. However, the fee is never re-applied to this new amount. Rational minters would always skip paying the fee by providing an allowance of 1 wei less than the required member token value.

HighOneTokens can be redeemed 1-to-1 for collateralFixed

The `redeem` function updates the `collateral` oracle price but does not consult it for a price. Instead, collateral is redeemed 1-to-1 but collateral could be off-peg. As `mint` consults the collateral oracle price, but `redeem` treats them as pegged, there is arbitrage potential and the collateral reserve could be emptied.

HighOneTokens can be redeemed for profit because of decimal issuesFixed

The `redeem` function takes an `amount` parameter reflecting oneTokens (18 decimals) to be redeemed. This amount is added to the user's **collateral** balance but the collateral token could have a different number of decimals, like USDT/USDC (6 decimals), netting a 10^12 profit and allowing to withdraw the whole reserve. ```solidity uint netTokens = amount.sub(amount.mul(redemptionFee).div(PRECISION)); // div by precision because of fee // increases collateral with oneToken amount increaseUserBalance(msg.sender, collateral, netTokens); ```

HighWrong precision returned in `Oracle.amountRequired`Fixed

It's not clear what precision the `amountUsd` argument of the `amountRequired` function is supposed to be. For `ICHIPeggedOracle`, it always returns the same precision as `amountUsd` which leads to a bug when used in this `OneTokenV1.mint` call: ```solidity (uint collateralTokensReq, /* volatility */) = IOracle(assets[collateralToken].oracle).amountRequired(collateralToken, collateralUSDValue); ``` `collateralUSDValue` is in `oneToken` precision (18 decimals) but when used with another stablecoin like USDT or USDC (6 decimals) the pegged oracle returns 10^12 times the stable coin amount actually required. The same issue exists for `UniswapOracleSimple.amountRequired` where it seems like `amountUsd` should be in `indexToken` precision to return the correct amount in `token`.

Medium`removeStrategy` does not close the strategy's positionFixed

There is no call to `closeStrategy(token)` in `removeStrategy`, which may cause tokens to be locked.

MediumIncorrect `amountRequired` functionFixed

The function appears to be copied from `ICHIPeggedOracle.sol`, however a 1:1 ratio between input and output tokens is not guaranteed in a composite oracle.

MediumMissing pair initializer check in `UniswapOracleSimple`Fixed

`UniswapOracleSimple.update` does not check if the pair has been initialized and will store wrong prices as the average prices will be computed since 1970. (time elapsed equals block timestamp.) The `pairInfo` function will return this wrong price, reading the price through `read`/`consult` will fail though.

Low`Incremental`'s events miss oneToken parameterFixed

`Incremental` allows changing settings for several oneTokens. However, its events don't indicate the `oneToken` the settings were changed for which makes it hard for a backend to associate settings changes with the `oneToken`.

Low`mintingRatio` might never reach its boundsFixed

The `mintingRatio` might never reach its min or max ratio as it is only changed by `stepSize` and not clamped, see `Incremental.getMintingRatio4`.

Low`setStrategy` ignores `allowance` parameterFixed

The function always sets the allowance to `INFINITE`: `IStrategy(strategy).setAllowance(token, INFINITE);`, regardless of the `allowance` parameter.

Show 10 more

Low`updateMintingRatio` can be called repeatedlyFixed

Although the function is only intended to make a small step to the ratio value, the function can be called repeatedly (even in the same transaction). This could potentially be used if an arbitrage attack could benefit from changing the ratio significantly beyond the step size.

LowApproval of old strategy is not revokedFixed

`OneTokenV1Base.closeStrategy` should revoke the approval of an old strategy (similar to how `setStrategy` sets approval) to shut down vulnerable strategies from causing havoc. The `@dev` comment talks about resetting the approval but it's not done: > @dev strategy remains assigned the asset with allowance set to 0.

LowContract size potentially exceeds the limitFixed

The contract `OneTokenV1.sol` appears to exceed 24576 bytes, and may have issues during deployment without further optimization.

LowIncompatible with non-standard ERC20 tokensFixed

The documentation mentions using stable coins as collateral but some common ERC20 stablecoins, like USDT, do not correctly implement the ERC20 specification as the functions don't return a boolean. This will lead to reverts when interacting with them, for instance here in `OneTokenV1.mint`: ```solidity IERC20(collateralToken).transferFrom ```

LowInconsistent constraints in `setMinRatio` and `setMaxRatio`Fixed

The function `setMinRatio` has the constraint `minRatio <= p.maxRatio`, whereas `setMaxRatio` uses `maxRatio > p.minRatio`. It is unclear from these two functions whether `maxRatio == p.minRatio` should be allowed.

LowMinting does potentially not update the member token oracleFixed

In `mint` there is a comment saying: ```solidity // this will also update the member token oracle price history (uint mintingRatio, uint maxOrderVolume) = updateMintingRatio(collateralToken); ``` This is not true unless `MintMaster.oneTokenOracles[oneToken]` always equals the member token oracle defined in the version contract `OneTokenV1.assets[memberToken].oracle)`. The oracle is not updated before performing the `IOracle(assets[memberToken].oracle).amountRequired(memberToken, memberTokensUSDValue)` call which could allow minting undercollateralized oneTokens and redeeming them for a profit.

LowMinting fees are only paid on collateral valueFixed

The minting fee in `mint` is only paid on the collateral value, not on the member token value, i.e., not on the whole oneToken value.

LowOracle `update` function not always called after initializationFixed

In `OneTokenFactory.sol`, the function `assignOracle` invokes the Oracle's `init` function followed by `update`. However, `admitForeignToken` only invokes `init`. A similar pattern occurs in `OneTokenV1Base.sol`, where `changeMintMaster` does not invoke `update` unlike in `init`.

LowSetter functions for min and max ratio may break step size invariantFixed

The function `setStepSize` ensures that it "must be smaller than the difference of min and max". However, `setMinRatio` and `setMaxRatio` do not have similar checks. If the difference is set too small, this invariant will no longer hold.

LowUnchecked function argumentsFixed

The following functions should perform additional checks on their arguments: 1. `OracleCommon.constructor` should check that `oneTokenFactory_` and `indexToken_` are non-zero. 1. In `UniswapOracleSimple.constructor`, `indexToken_` and `period_` should be non-zero. This would also handle the same missing check in `ICHICompositeOracle.constructor`. 1. `ICHIModuleCommon.updateDescription` does not check if `description` length is non-zero, but the constructor does. 1. `Incremental._updateMintingRatio` should check `p.set`. 1. `OneTokenV1Base.setStrategyAllowance` should check if `assets[token].strategy` exists. Otherwise, a null strategy is approved. 1. `OneTokenFactory.assignOracle` does not check `isModule(oracle)` but `admitForeignToken` does.

Show 1 more

LowUnclear distinction between `execute` and `executeTransaction`Fixed

`StrategyCommon` declares a virtual function `execute`, whereas `Arbitrary` declares `executeTransaction`. It is further unclear how `Arbitrary.executeTransaction` relates to `OneTokenV1Base.executeStrategy`, which invokes `IStrategy(strategy).execute();`. If the strategy is of type `Arbitrary`, it appears that this function call will do nothing. Note that `OneTokenV1Base.executeStrategy` also emits a `StrategyExecuted` event, so there may be duplicate events emitted. While the `Arbitrary.executeTransaction` function does not emit a `StrategyExecuted` event, the inherited `execute` function does.