RariCapital V2

Quantstamp20 Aug 2020Files named in the report

Quantstamp reviewed Rari Capital (RariCapital V2) on 20 Aug 2020: 41 findings, 0 critical; 18 marked fixed, 18 acknowledged, 5 open.

No open critical findings

Open the report Open Rari Capital’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings41

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

Severity	Open	Acknowledged	Fixed
Critical	0	0	0
High	0	0	3
Medium	1	1	4
Low	4	17	11

MediumDivergent mirrored statesAcknowledged

There are several state variables that are mirrored in the following contracts: `RariFundManager`, `RariFundController` and `RariFundProxy`, namely: 1. `_fundDisabled`: Boolean that, if true, disables the primary functionality of the contract. 1. `_rariFundRebalancerAddress`: Address of the rebalancer. 1. `_supportedCurrencies`: Array of currencies supported by the fund. 1. `_erc20Contracts`: Maps ERC20 token contract addresses to supported currency codes. 1. `_currencyDecimals`: Maps decimal precisions (number of digits after the decimal point) to supported currency codes. 1. `_poolsByCurrency`: Maps arrays of supported pools to currency codes. During development (before deployment), this creates ambiguity which makes maintainability difficult and error prone, because developers: (1) might forget to update all the values of these state variables in all contracts they occur or they (2) might update the state variables with different values in different contracts. For example if new supported currencies are added any of the following input parameters could be set differently for different contracts: `currencyCode`, `erc20Contract`, `decimals` and `pool`. This would have a significant impact on the system as a whole. <br> After deployment the value of: 7. `_fundDisabled` can be set independently in different contracts by calling the `disableFund` and `enableFund` functions, which could lead the fund to be disable in one contract and enabled in the other contract. This can have an important impact on deposits, withdrawals, orders and/or approvals performed by end-users, when values are set differently during the small time window in which the 2 separate function calls are performed. 8. `_rariFundRebalancerAddress` can be set independently in different contracts by calling the `setFundRebalancer` function. This can have an important impact on deposits, withdrawals, orders and/or approvals performed by end-users, when values are set differently during the small time window in which the 2 separate function calls are performed.

LowBlock Timestamp ManipulationAcknowledged

Projects may rely on block timestamps for various purposes. However, it's important to realize that miners individually set the timestamp of a block, and attackers may be able to manipulate timestamps up to 900 seconds, for their own purposes. If a smart contract relies on a timestamp, it must take this into account. The `checkLossRateLimit` makes a decision based on the block timestamp on L537. However, the interval there seems to be 24 hours, which is a far larger than 900 seconds. Therefore, the attacker can only benefit by stopping the iteration of the for loop earlier than expected and use another value for `lossRateLastDay` than intended by the developer.

LowController unable to pause specific stablecoinsAcknowledged

Stablecoins, like other financial assets on-chain, are likely victims of recurrent attacks or attempted attacks. In case of specific disruption of any stablecoin, the controllers of the pools have no functionality to selectively pause or stop the use of particular stablecoin.

LowETH/USD prices could be staleAcknowledged

The following functions do not check if the ETH/USD price is stale: 1. `RariGovernanceTokenDistributor.getEthUsdPrice` in `rari-governance-contracts` 1. `RariFundPriceConsumer.getDaiUsdPrice` in `rari-stable-pool-contracts` and `rari-yield-pool-contracts` 1. `RariFundPriceConsumer.getEthUsdPrice` in `rari-stable-pool-contracts` and `rari-yield-pool-contracts` 1. `RariFundPriceConsumer.getPriceInEth` in `rari-stable-pool-contracts` and `rari-yield-pool-contracts`. According to the Chainlink documentation: * [under current notifications](https://docs.chain.link/docs/developer-communications#current-notifications): "if answeredInRound < roundId could indicate stale data." * [under historical price data](https://docs.chain.link/docs/historical-price-data#solidity): "A timestamp with zero value means the round is not complete and should not be used."

LowExpired cacheAcknowledged

The functions `_depositTo`, `_withdrawFrom`, and `withdrawFees` in `RariFundManager.sol@rari-ethereum-pool-fund` do not update `_rawFundBalanceCache` at all, which is different from the behavior of the same functions in the other repositories: `rari-staple-pool-contracts` and `rari-yield-pool-contracts`.

LowFallback function can receive funds from any address (2)Acknowledged

The fallback function is meant to only be “called by 0x exchange to refund unspent protocol fee.” However, there are no restrictions/checks in place to guarantee this. This means that anyone could send funds to this contract by mistake.

LowHardcoded dependency contractsAcknowledged

Controllers of external contracts have hardcode addresses. In the extreme scenario where these contracts are replaced by new ones (hard upgrade) or the community capital migrates to similar ones (code fork), there is no way to upgrade the addresses of the controlled contracts.

LowIncreased loss of precision due to dividing before multiplicationAcknowledged

To reduce the loss of precision caused by integer division, multiplication should always be performed before division. Several locations in the code were identified where this rule is not satisfied and hence a larger loss of precision is possible: 1. In `RariFundProxy.withdrawAndExchange` the division in the following assignment `uint256 outputAmount = 18 >= outputDecimals ? inputAmounts[i].div(10 ** (uint256(18).sub(outputDecimals))) : inputAmounts[i].mul(10 ** (outputDecimals.sub(18)));` is performed before the multiplication in this assignment `realOutputAmount = outputAmount.sub(outputAmount.mul(MStableExchangeController.getSwapFee()).div(1e18));` 1. In `MStablePoolController.withdraw` the division in the following assignment `uint256 credits = amount.mul(1e18).div(exchangeRate);` is performed before the division in the following `if`-condition `if (credits.mul(exchangeRate).div(1e18) < amount)` 1. In `RariFundManager.depositTo` the division in the following assignment `uint256 amountUsd = amount.mul(pricesInUsd[_currencyIndexes[currencyCode]]).div(10 ** _currencyDecimals[currencyCode]);` is performed before the multiplication in the following assignment `rftAmount = amountUsd.mul(rftTotalSupply).div(fundBalanceUsd);` 1. In `RariFundPriceConsumer.getMUsdUsdPrice` the following assignment contains a division before the last multiplication `usdSupplyScaled = usdSupplyScaled.add(bAssets[i].vaultBalance.mul(bAssets[i].ratio).div(1e8).mul(bAssetUsdPrices[i]));` 1. In `RariGovernanceTokenDistributor.storeRgtDistributedPerRft` the following assignment contains a division before the last multiplication `_rgtPerRftAtLastSpeedUpdate[i_scope_0] = _rgtPerRftAtLastSpeedUpdate[i_scope_0].add(rgtToDistribute.mul(ethFundBalanceUsd).div(fundBalanceSum).mul(1e18).div(totalSupply))` 1. In `RariGovernanceTokenDistributor.storeRgtDistributedPerRft` the following assignment contains a division before the last multiplication `_rgtPerRftAtLastSpeedUpdate[i_scope_0] = _rgtPerRftAtLastSpeedUpdate[i_scope_0].add(rgtToDistribute.mul(_fundBalancesCache[i_scope_0]).div(fundBalanceSum).mul(1e18).div(totalSupply))` 1. In `RariGovernanceTokenDistributor.getRgtDistributedPerRft` the following assignment contains a division before the last multiplication `_rgtPerRftAtLastSpeedUpdate[uint8(pool)].add(rgtToDistribute.mul(ethFundBalanceUsd).div(fundBalanceSum).mul(1e18).div(totalSupply))` 1. In `RariGovernanceTokenDistributor.getRgtDistributedPerRft` the following assignment contains a division before the last multiplication `_rgtPerRftAtLastSpeedUpdate[uint8(pool)].add(rgtToDistribute.mul(_fundBalancesCache[uint8(pool)]).div(fundBalanceSum).mul(1e18).div(totalSupply)) ` 1. In `RariGovernanceTokenDistributor.getRgtDistributedPerRft` the following assignment contains a division before the last multiplication `rgtPerRftByPool[i_scope_0] = _rgtPerRftAtLastSpeedUpdate[i_scope_0].add(rgtToDistribute.mul(ethFundBalanceUsd).div(fundBalanceSum).mul(1e18).div(totalSupply))` 1. In `RariGovernanceTokenDistributor.getRgtDistributedPerRft` the following assignment contains a division before the last multiplication `rgtPerRftByPool[i_scope_0] = _rgtPerRftAtLastSpeedUpdate[i_scope_0].add(rgtToDistribute.mul(_fundBalancesCache[i_scope_0]).div(fundBalanceSum).mul(1e18).div(totalSupply))`

LowMissing input argument validation (2)Acknowledged

The following functions are missing input parameter validation: 1. `RariFundController.setFundManager` in `rari-ethereum-pool-fund` does not validate the `newContract` parameter of type `address`. 1. `setFundRebalancer` in all repos and all contracts does not check the `newAddress` parameter of type `address`. 1. `setFundPriceConsumer` in all repos does not check the `newContract` parameter of type `address`.

LowMissing input validationAcknowledged

The following function should perform additional checks: 1. `RariFundController.setEnzymeComptroller` should ensure that `comptroller` is non-zero. 1. `RariFundController.marketSell0xOrdersFillOrKill` should ensure that `inputErc20Contract` is non-zero. 1. `RariFundController.setFundManager` should ensure that `newContract` is non-zero. 1. `RariFundController.setFundRebalancer` should ensure that `newAddress` is non-zero. 1. `RariFundManager.authorizeFundManagerDataSource` should ensure that `authorizedFundManagerDataSource` is non-zero. 1. `RariFundManager.setFundController` should ensure that `newContract` is non-zero. 1. `RariFundManager.setFundToken` should ensure that `newContract` is non-zero. 1. `RariFundManager.setFundProxy` should ensure that `newContract` is non-zero. 1. `RariFundManager.setFundRebalancer` should ensure that `newContract` is non-zero.

Show 8 more

LowPotential funds stuck in contractAcknowledged

In `withdrawAndExchange()`, does there need to be a check that all orders obtain tokens of the same type (corresponding to `outputErc20Contract`). For example, suppose one order obtained WETH and another contained DAI, and `outputErc20Contract = address(0)`. Wouldn't the DAI funds be stuck in the contract until another `withdrawAndExchange()` transaction occurs with `outputErc20Contract = DAI`?

LowPrivileged Roles and OwnershipAcknowledged

Smart contracts will often have `owner` variables to designate the person with special privileges to make modifications to the smart contract. There are multiple privileged roles in the system, including: contract owners, rebalancers and Rari fund managers/controllers. 1. The owner of the `RariFundController` contract is allowed to: * disable and enable the Rari fund at any point in time. * set the daily loss rate limit to any value at any time. * forward all funds in the contract to any EOA. * change the `RariFundToken` and `RariFundProxy` address at any time. 1. The Rari Fund rebalancer is allowed to: * withdraw all funds from any and all pools at any time. * approve any amount to 0x exchange. * create sell orders on the 0x exchange. 1. The owner of the `RariFundManager` contract is allowed to withdraw all funds (of any token type, including ETH) out of this smart contract to their own account.

LowPrivileged Roles and Ownership (2)Acknowledged

1. The minter of the `RariFundToken` is allowed to set the `rariGovernanceTokenDistributor` address to any value at any point in time (even if the new `rariGovernanceTokenDistributor` is disabled) if the `force` parameter is set to `true`. It is not clear how, when or why the `force` parameter would be used in `setGovernanceTokenDistributor()`to prevent reverting if the validation checks existent in that function would fail. 1. The owner of the `RariGovernanceTokenDistributor` contract can: * Enable and disable the distribution at any time, multiple times. * Set the governance token, fund token and fund manager addresses to any non-zero address when the distribution is disabled. * Upgrade the contract address to any address, which transfers all RGTs to that address. 1. The owner of `RariFundController` can set the address of the `_rariFundManagerContract` to any address including a EOA and then use that address to withdraw all the funds from all pools using the `withdrawToManager` and/or `withdrawFromPoolKnowingBalanceToManager` functions. 1. The owner of the `RariFundManager` can: * Upgrade the fund manager contract. * Authorize any address to be the fund manager data source. * Set the fund controller, fund proxy, fund rebalancer and fund token to any address. * Set the interest fee rate to values even higher than 100%. * Set the interest fee master beneficiary to any address different from zero.

LowRari Governance Tokens can still be claimed after distribution endsAcknowledged

There is a recurring condition that appears in 6 methods inside the `RariFundToken` contract, namely: `if (address(rariGovernanceTokenDistributor) != address(0) && block.number > rariGovernanceTokenDistributor.distributionStartBlock())`, which appears in the following functions: `transfer`, `transferFrom`, `mint`, `burn`, `burnFrom`and ` fundManagerBurnFrom`. This condition does not check whether the current block number is past the end block of the distribution.

LowSetter function missing eventAcknowledged

Although the other setters are emitting events, the setter ‘setFundManagerData()’ has no corresponding event declared and is not emitting any event.

LowSingle point of failure for price feedsAcknowledged

The price feeds rely on a single oracle, namely the Chainlink Aggregator V3, which is indeed robust. However, in the event of any large scale attack/disruption of the Chainlink network, Rari Capital would be impacted severely.

LowUnexpected poolAcknowledged

In `RariGovernanceTokenDistributor.sol@rari-governance-contracts`, the functions `setFundManager`, `setFundToken`, `beforeFirstPoolTokenTransferIn`, `getUnclaimedRgt`, `_claimRgt`, `claimRgt` and `refreshDistributionSpeeds` have an input parameter called `pool` of type `RariPool`, which is an `enum` with 3 values. When end-users call these functions they will be able to pass in an integer value for this parameter, which could be higher than `2`, which is the highest value allowed by the `enum`. This will cause the function to throw without any explicit error message and might be confusing to the end-user as to why the function reverted.

LowUpgrading Fund Controller can be done when fund is enabledAcknowledged

In `RariFundController.upgradeFundController()`function in both the `rari-ethereum-pool-fund` and `rari-stable-pool-contracts` repos, it is not required that the fund is disabled, unlike the same function in the `rari-yield-pool-contracts` repo. It is not clear if this is intentional or not.