Secured Finance security record

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.

The function `LendingMarketOperationLogic.createZCToken()` is used to deploy two types of ZC Tokens: perpetual and non-perpetual. * Non-perpetual ZC Tokens for an underlying token `T` should have a number of decimals that match the amount of decimal of `T`. * Perpetual ZC Tokens for this same underlying token `T` should have a number of decimals that matches the number of decimals of the genesis value of `T`, which should have 18 additional decimals. However, all ZCTokens are deployed with 18 additional decimals to match the genesis value amount. As a result, users withdrawing non-perpetual ZC Tokens may think that they received 1e18 times less than expected. **Note:** This issue was raised by the client during the audit process.

Users have the capability to withdraw a specified `_amount` of ZC tokens using the `_withdrawZCToken()` and `_withdrawZCPerpetualToken()` functions. However, the presence of the `isAll` flag, as determined by the `_getWithdrawableZCTokenAmount()` function, indicates that users can withdraw their entire position for a specific maturity. This functionality effectively eliminates the option for partial withdrawals. Specifically, based on the `isAll` value, either `0` or the `_amount` specified will be forwarded to the `lock()` function. The `lock()` function interprets an amount of `0` as an instruction to lock the user's total balance or the specified amount. Consequently, if `isAll` is set to `true` for a user intending to withdraw only `8` out of `10` tokens, all `10` tokens will be locked, not just the `8` requested. This could inadvertently impact the user's risk management strategies, as it differs from their initial withdrawal intent. **Note:** This issue was raised by the client during the audit process.

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()`.