Merge pull request #20 from Zellic/data_update_november

Data update november

.gitmodules

@@ -4,3 +4,6 @@
 [submodule "pdfs/dedaub-audits"]
 	path = pdfs/dedaub-audits
 	url = https://github.com/Dedaub/audits
+[submodule "pdfs/publications"]
+	path = pdfs/publications
+	url = https://github.com/trailofbits/publications.git

findings_newupdate/dedaub/GMX _ GMX Audit - Oct '22.txt

@@ -0,0 +1,13 @@
+C1 Reentrancy vulnerability in cancelOrde STATUS OPEN OrderHandler::cancelOrder, which is an external function, is not protected by a reentrancy guard. Moreover, OrderUtils::cancelOrder (which performs the actual operation) transfers funds to the user (orderStore.transferOut) before updating its state. As a consequence, a malicious adversary could re-enter in cancelOrder, and execute an arbitrary number of transfers, eectively draining the contracts full balance in the corresponding token. function cancelOrder( DataStore dataStore, EventEmitter eventEmitter, OrderStore orderStore, bytes32 key, address keeper, uint256 startingGas ) internal { Order.Props memory order = orderStore.get(key); validateNonEmptyOrder(order); if (isIncreaseOrder(order.orderType()) || isSwapOrder(order.orderType())) { if (order.initialCollateralDeltaAmount() > 0) { orderStore.transferOut( EthUtils.weth(dataStore), order.initialCollateralToken(), order.initialCollateralDeltaAmount(), order.account(), order.shouldConvertETH() ); } } // Dedaub: state changed after the transfer, also idempotent orderStore.remove(key, order.account());  Note that the main re-entrancy method, namely the receive hook of an ETH transfer, is in fact protected by using payable(receiver).transfer which limits the gas available to the adversarys receive hook. Nevertheless, an ER
+C20 token transfer is an external contract call and should be assumed to potentially pass the execution to the adversary. For instance, an ERC777 token (which is ERC20 compatible) would implement transfer hooks that could easily be used to perform a reentrancy aack. Note also that the state update (orderStore.remove(key, order.account())) is idempotent, so it can be executed multiple times during a reentrancy aack without causing an error. To protect against reentrancy we recommend 1. Adding reentrancy guards, and 2. Execute all state updates before external contract calls (such as transfers). Note that (2) by itself is suicient, so reentrancy guards could be avoided if gas is an issue. In such a case, however, comments should be added to the code to clearly state that updates should be executed before external calls, to avoid a vulnerability being reintroduced in future restructuring of the code. HIGH SEVERITY: ID Description STATUS 
+H1 Conditional execution of orders using shouldConvertETH OPEN For all order types that involve ETH, a user can set the option shouldConvertETH =true to indicate that he wishes to receive ETH instead of WETH. Although convenient, this gives an adversary the opportunity to execute conditional orders in a very easy way. The adversary can simply use a smart contract as the receiver of the order, and set a receive function as follows:  contract Adversary { bool allow_execution = false; receive() { require(allow_execution); } } Then, in the time period between the order creation and its execution, the adversary can decide whether he wishes the order the allow_execution variable accordingly. If unset, the receive function will revert and the protocol will cancel the order. to succeed or not, and set The possibility of conditional executions could be exploited in a variety of dierent scenarios, a concrete example is given at the end of this item. Note that the use of payable(receiver).transfer (in Bank::_transferOutEth) does not protect against this aack. The 2300 gas sent by transfer are enough for a simple check like the one above. Note also that, although the case of ETH is the simplest to exploit, any tokens that use hooks to allow the receiver to reject transfers (eg ERC777) would enable the same aack. Note also that, if needed, the time period between creation and execution could be increased by simultaneously submiing a large number of orders for tiny amounts (see L2 below). One way to protect against conditional execution is to employ some manual procedure for recovering the funds in case of a failed execution (for instance, keeping the funds in an escrow account), instead of simply canceling the order. Since a failed execution should not happen under normal conditions, this would not aect the protocols normal operation. Concrete example of exploiting conditional executions: The adversary wants to take advantage of the volatility of ETH at a particular moment, but without any trading risk. Assume that the current price of ETH is 1000 USD, he proceeds as follows:  - He creates a market swap order A to buy ETH at the current price. In this order, he sets shouldConvertETH = true and the receive function above that conditionally allows the execution. - He also creates a limit order B to sell ETH at 1010 USD. He then monitors the price of ETH before the orders execution: - If ETH goes down, he does nothing. allow_execution is false so order A will fail, and order B will also fail since the price target is not met. - If ETH goes up, he sets allow_execution = true, which leads to both orders succeeding for a prot of 10 USD / ETH. 9 MEDIUM SEVERITY: ID Description 
+M1 Incorrect handling of rebalancing token STATUS OPEN StrictBank::recordTokenIn computes the number of received tokens by comparing the contract's current balance with the balance at the previous execution. However, this approach could lead to incorrect results in the case of ER
+C20 tokens with non-standard behavior, for instance: - Tokens in which balances automatically increase (without any transfers) to include interest. - Tokens that allow interacting from multiple contract addresses To be more robust with respect to such types of tokens, we recommend comparing the balance before and after the current incoming transfer, and not between dierent transactions. 
+M2 Self-close bad debt aack OPEN The protocol is susceptible to an aack involving opening a delta neutral position using Sybil accounts, with maximum leverage, on a volatile asset. Scenario: 1. Aacker controls Alice and Bob 2. Alice opens a large long position with maximum leverage 3. Bob opens a large short position with maximum leverage 4. Market moves 5. Alice liquidates their underwater position, causing bad debt 6. Bob closes their other position, proting on the slippage The reason why this aack is possible is that using the current design, it takes multiple blocks for a liquidator to react and by that time their order is executed it is possible that one of the positions is underwater. Secondly, when liquidating, Alice does not suer a bad price from the slippage incurred in the liquidation but Bob benets from the 1 slippage, when closing their position just after. Another factor that contributes towards this aack is that the liquidation penalty is linear, while the price impact advantage is higher-order, making the aack increasingly protable the larger the positions. To deter this, the protocol could support (i) partial liquidations for large positions and therefore force the positions to be closed gradually, making the aack non-viable, and, (ii) slippage open-interest calculations used to determine the price of the liquidation. LOW SEVERITY: 
+L1 Missing receive function OPEN OrderStore does not contain a receive function so it cannot receive ETH. However, this is needed by Bank::_transferOutEth, which withdraws ETH before sending it to the receiver. function _transferOutEth(address token, uint256 amount, address receiver) internal { require(receiver != address(this), "Bank: invalid receiver"); IWETH(token).withdraw(amount); payable(receiver).transfer(amount); _afterTransferOut(token); } WIthout a receive function any transaction with shouldConvertETH = true would fail. 1 
+L2 No lower bounds for swaps and positions OPEN Since there are no lower bounds for the size of a position, someone could in principle create a large number of tiny orders. Such a strategy would cost the adversary only the gas fees and the total amount of the requested positions, which can be as small as he wishes. In this 2-step procedure used in this version of the protocol, such a behavior could potentially create a problem, because the order keepers would have to execute a huge number of orders. We suggest seing a minimum size for positions and swaps. L3 Inconsistency in calculating liquidations OPEN The comment in PositionUtils::isPositionLiquidatable indicates that price impact is not used when computing whether a position is liquidatable. However, the price impact is in fact used in the code: // price impact is not factored into the liquidation calculation // if the user is able to close the position gradually, the impact // may not be as much as closing the position in one transaction function isPositionLiquidatable( DataStore dataStore, Position.Props memory position, Market.Props memory market, MarketUtils.MarketPrices memory prices ) internal view returns (bool) { ... int256 priceImpactUsd = PositionPricingUtils.getPriceImpactUsd(...)  int256 remainingCollateralUsd = collateralUsd.toInt256() + positionPnlUsd + priceImpactUsd + fees.totalNetCostAmount; 1 On the other hand, when the liquidation is executed, the price impact is not used. The comment in DecreasePositionUtils::processCollateral indicates that this is intentional: // the outputAmount does not factor in price impact // for example, if the market is ETH / USD and if a user uses USDC to long ETH // if the position is closed in profit or loss, USDC would be sent out from or // added to the pool without a price impact // this may unbalance the pool and the user could earn the positive price impact // through a subsequent action to rebalance the pool // price impact can be factored in if this is not desirable If this inconsistency is intentional, it should be properly documented in the comments. Note that, when deciding on a liquidation strategy, you should have in mind the possibility of cascading liquidations, namely the possibility that executing a liquidation causes other positions to become liquidatable. CENTRALIZATION ISSUES: It is often desirable for DeFi protocols to assume no trust in a central authority, including the protocols owner. Even if the owner is reputable, users are more likely to engage with a protocol that guarantees no catastrophic failure even in the case the owner gets hacked/compromised. We list issues of this kind below. (These issues should be considered in the context of usage/deployment, as they are not uncommon. Several high-prole, high-value protocols have signicant centralization threats.) 1 ID Description 
+N1 Order keepers can cause DoS in all operations OPEN Due to the two step execution system, any operation requires an order keeper to execute it. Trust is needed in that order keepers will timely execute all pending orders. If the order keeper does not submit execution transactions, all operations will cease to function, including closing positions and withdrawing funds. It would be benecial to implement fallback mechanisms that guarantee that users can at least withdraw their funds in case order keepers cease to function for any reason. For instance, the protocol could allow users to execute orders by providing oracle prices themselves, but only if an order is stale (a certain time has passed since its creation). N2 Order keepers can frontrun/reorder transactions There is nothing in the current system that prevents an order keeper from front-running or reordering transactions, for instance to exploit changes in the price impact. The protocol could include mechanisms that limit this possibility: for instance, the order keeper could be forced to execute orders in the same order they were created. OTHER / ADVISORY ISSUES: This section details issues that are not thought to directly aect the functionality of the project, but we recommend considering them. ID Description 
+A1 Possible erroneous computation of price impact STATUS INFO 1 imbalance) ^ (price impact exponent) * (price impact factor) - (next Price impact is calculated as (initial imbalance) ^ (price impact exponent) * (price impact factor) The values of exponent (e) and impact factor (f) are set by the protocol for each market. If the impact factor is simply a percentage, then the price impact will have units USD^(price impact exponent). But this seems erroneous, since price impact is treated as a USD amount which is nally added to the amount requested by the user. A problem arises in case that these two quantities are selected independently of each other but also of the pool's deposits and status. For example, consider a pool with tokens A and B of total USD value x and y respectively. Consider that x < y. Then the imbalance equals d = y - x. If a user swaps A tokens of worth d/2, then prior to the price impact he will get B tokens of the same value. The new deposits of the pool will now be x'=y'=(x+y)/2 and the pool will become balanced. The price impact for this transaction is f*d^e, which could be ( if the parameters are not chosen carefully) larger than d/2, which is the requested swap amount. Also, this fact could lead to a pool which is even more imbalanced than the previous state. We suggest that (total_deposits)^(e-1)*f always be less than 1 to avoid the above mentioned undesirable behavior. 
+A2 Inconsistency in the submission time of dierent tokens INFO Price keepers are responsible for submiing prices for most of the protocol's tokens. The submied price should be the price retrieved from exchange markets at the time of each order's creation (the median of all these prices is nally used). However, for some tokens Chainlink oracles are used. In this case, the price at the time of the order execution is used. 
+A3 A user can liquidate its own position INFO there is nothing preventing a user ExchangeRouter::createLiquidation can be called only by liquidation keepers. However, from calling createOrder with orderType=Liquidation, eectively creating a liquidation order for their own position. Although this is not necessarily an issue, it is unclear whether this functionality is intentional or not. 1 
+A4 Ineicient price impacts on large markets INFO The price impact calculation is a function with an exponential factor of the absolute dierences between open long and short interests before and after a trade. This works well for the average market, but on a large market with large open interests, it is not more eicient to open large positions. Consider that in other AMM designs, it is possible to open large positions with minimal price impact if the market is large (e.g., Uniswap ETH-USDC). A5 Known compiler bugs INFO The code can be compiled with Solidity 0.8.0 or higher. For deployment, we recommend no floating pragmas, but a specic version, to be condent about the baseline guarantees oered by the compiler. Version 0.8.0, in particular, has some known bugs, which we do not believe aect the correctness of the contracts. 1