Secured Finance

Several contracts use a list of names (`acceptedContracts`) as well as a modifier `onlyAcceptedContracts` to restrict some of their functions to only a subset of addresses. The contract `TokenVault` that "accepts" the contracts `LendingMarketController` and `ReserveFund`. In parallel, the contract `ReserveFund` inherits the contract `MixinWallet`, which lets the owner of that contract execute arbitrary calls to arbitrary addresses with arbitrary payloads via the functions `MixinWallet.executeTransaction()` and `MixinWallet.executeTransactions()`. As a consequence, the owner of `ReserveFund` can execute the following restricted functions of the contract `TokenVault`: * `depositFrom()` where any available amount of token allowed to be spent by the protocol can be transferred from users' wallets to the protocol; * `addDepositAmount()` where the deposit balance of a registered currency of any user can be increased by any amount; * `removeDepositAmount()` where the deposit balance of a registered currency of any user can be decreased by any available amount; * `executeForcedReset()` where the deposit balance of a registered currency of any user can be set to 0; * `transferFrom()` where the deposit balance of a registered currency of any user can be transferred to any other user; After executing the exploit scenario described below, one address with a non-zero balance can withdraw funds from the system via the function `withdraw()`. The root cause of this issue is the fact that the access control mechanism based on a list of `acceptedContracts` does not respect the Principle of Least Privilege. Several contracts requiring access to restricted functions of a third contract for separate use cases both obtain access to all restricted functions of that third contract.

`ReserveFund` contract implements a pausing system through a boolean variable `paused` and a public function `isPaused()` that returns it. Functions `pause()` and `unpause()` toggle `paused` to `true` and `false`. Functions `pause()` and `unpause()` are both unprotected by any access modifiers. This allows an attacker to pause and unpause the `ReserveFund` contract as they please. Whether the contract is paused is checked by `LiquidationLogic.executeLiquidation()` and depending on the result, different calculations are made to cover insolvent positions, which may allow an attacker to force improper calculations while their positions are being liquidated by front-running the liquidation transaction to pause/unpause the contract.

A liquidator may be able to repay a position either in a token that used to be accepted as collateral by the protocol or a token that used to be accepted for trading. When a liquidator calls `executeForcedRepayment()`, the function attempts forced repayment on behalf of a user if the user defaulted on a borrowing position. However, the function performs no validation on the supplied `_collateralCcy`, allowing repayment to take place even if the collateral currency is no longer accepted by the protocol, whether as collateral or as a trading token. This is possible because the `TokenVault` is responsible for validating whether a currency is allowed as collateral. However, when `LiquidationLogic._transferCollateral()` calls `TokenVault.transferFrom()`, which is protected by the `onlyRegisteredCurrency` modifier, the check in `isRegisteredCurrency()` only validates that `Storage.slot().tokenAddresses[_ccy] != address(0)`. This will irreversibly be true for any currency that is added via `TokenVault.registerCurrency()` since the function call adds the passed address to the `tokenAddresses` mapping, however calling `TokenVault.updateCurrency()` will only remove the `_ccy` entry from `collateralCurrencies`. Additionally, during the function's execution, it calls `CurrencyController.convert()` to get the `liquidationAmountInCollateralCcy`. The function call then lands on `CurrencyController._getAggregatedLastPrice()`, which will return `1` if the currency has no pricefeeds or the actual price of the currency in case it was previously accepted as collateral (see `Haircut and pricefeed data not erased when a currency is removed` for more context).

The Secured Finance protocol implements a lazy evaluation logic, where certain data structures are left unupdated after specific operations to mitigate gas costs. Notably, in the execution of an order, the array containing a user's active (unfilled) orders for a specific order book is not updated within the transaction. This array is only updated in the subsequent transaction before any user-related operations. The critical function at the center of this issue is `LendingMarket.cleanUpOrders()`. This function calls `OrderActionLogic.cleanUpOrders()`, responsible for updating the unfilled orders of a user for a given order book, specifically `orderbook.activeLendOrderIds[user]` and `orderbook.activeBorrowOrderIds[user]`. This function edits these arrays and expects that the caller will update the removed funds. If not, these funds are not moved and will be lost. The function `FundManagementLogic._cleanUpOrders()` shows how the different vaults should be updated with the amounts returned by `LendingMarket.cleanUpOrders()`. Importantly, this function lacks protection, allowing anyone to call it. While it successfully updates those arrays based on the order book trees, the funds filled of the removed orders from the array are not appropriately tracked or accounted for.

Each time an order is executed, its unit price is saved in the contract. This will be used to calculate the auto-roll price for the next market when the current market is rotated. The function `updateOrderLogs()` is called when an order is executed (at the moment of updating the taker's balance). In a liquid and active market, it will save the present and future value of all orders in the last 6 hours of the market (observation period, starting 6 hours before maturity). This way, the volume-weighted average unit price can be calculated later. However, to protect against the scenario of a market with no order executions in this observation period, the system also saves the `estimatedAutoRollUnitPrice`: the last executed price (converted to the new maturity). This price is recorded without verification: a small order of a few wei can set the next auto-rolling price. The system also implements data structures to save the weighted average unit price on a block, more secure and precise as it performs additional checks like a minimum reliable amount (minimum amount traded in a block to avoid price manipulation).

The `LendingMarketOperationLogic.executeEmergencyTermination()` function loops over the currencies in the protocol and invokes `pauseLendingMarkets()` on each of them, which looks like the following: ``` function pauseLendingMarkets(bytes32 _ccy) public { ILendingMarket market = ILendingMarket(Storage.slot().lendingMarkets[_ccy]); market.pauseMarket(); } ``` However, this function call will revert if a currency has no market or if the market is already paused (`_pause()` has `whenNotPaused` modifier), reverting the emergency termination trigger.

During the Itayose call, we calculate the `totalSupply` of Future Value (FV) using the Present Value (PV) (total amount filled in Itayose pre-orders) and `openingPrice` calculated from them. Following the scenario below: If the offset pre-order has the following amount with an opening unit price of `9220`, a precision loss will happen. ``` A: 100 B: 200 ``` After the Itayose process, the total supply will be `325 = (300 * 10000) / 9220`, calculated from the opening price and the total filled amount from the pre-orders. But each PV of order becomes: ``` A: 108 = (100 * 10000) / 9220 B: 216 = (200 * 10000) / 9220 ``` <br> This total sum of orders' future value is `324`, so the precision loss is `1` (`totalSupply` was set to `325`). `isAllRemoved` will never be fulfilled in `FutureValueVault.reset()`, leading to never calling `GenesisValueVault.updateGenesisValueWithResidualAmount()`. <br> ``` isAllRemoved = Storage.slot().removedLendingSupply[maturity] == Storage.slot().totalSupply[maturity] && Storage.slot().removedBorrowingSupply[maturity] == Storage.slot().totalSupply[maturity]; ```

In case of an emergency, a termination mechanism can be executed by the owner of the contract `LendingMarketController` to cease the operations of the protocol. Once executed, this operation cannot be reverted. For each active currency, the last aggregated price returned by the oracle is stored in the mapping `marketTerminationPrices`. Values in this mapping have the number of decimals stored in the mapping `CurrencyController.decimalCaches[]`. Then, users can force a settlement of all lending and borrowing positions via the function `executeEmergencySettlement()`. Once the protocol is terminated, it is possible to update the values in the mapping `CurrencyController.decimalCaches[]`, which would affect the prices used during the settlement process and obtained via the functions `FundManagementLogic._convertToBaseCurrencyAtMarketTerminationPrice()` and `FundManagementLogic._convertFromBaseCurrencyAtMarketTerminationPrice()`.

Several functions in the contract `TokenVault` only accept to work for currencies registered in the mapping `tokenAddresses`, based on the modifier `onlyRegisteredCurrency()`. The owner of the contract can register new currencies via the function `registerCurrency()`. Only currencies with an existing identifier registered in the contract `CurrencyController` can be registered. However, the fact that an address matches a given currency identifier is not checked. Also, the association between a currency identifier and an address can be updated by calling twice the function `registerCurrency()` for the same identifier by a different address. If the owner of the contract gets compromised, the exploit scenario below becomes possible.

1. Addresses with the role `OPERATOR_ROLE` can pause and unpause `LendingMarkets` if required. According to the logic in the contract `MixinAccessControl`, addresses with the role `DEFAULT_ADMIN_ROLE` can add or remove operators via the functions `addOperator()` and `removeOperator()`. However, it is possible to temporarily remain without any address having the role `OPERATOR_ROLE`, which would make it impossible to pause a `LendingMarket` in case of emergency. 2. Addresses with the role `DEFAULT_ADMIN_ROLE` can assign or revoke the roles `OPERATOR_ROLE` and `DEFAULT_ADMIN_ROLE`. However, it is possible to remain forever without any address having the role `DEFAULT_ADMIN_ROLE`, which would make it impossible to update the current assignments of Operators.