Ether temporarily held during transactions can be stolen via reentrancy
The exchange proxy typically holds no ether balance, but it can temporarily hold a balance during a transaction. This balance is vulnerable to theft if the following conditions are met:\\nNo check at the end of the transaction reverts if ether goes missing,\\nreentrancy is possible during the transaction, and\\na mechanism exists to spend ether held by the exchange proxy.\\nWe found one example where these conditions are met, but it's possible that more exist.\\nExample\\n`MetaTransactionsFeature.executeMetaTransaction()` accepts ether, which is used to pay protocol fees. It's possible for less than the full amount in `msg.value` to be consumed, which is why the function uses the `refundsAttachedEth` modifier to return any remaining ether to the caller:\\n```\\n/// @dev Refunds up to `msg.value` leftover ETH at the end of the call.\\nmodifier refundsAttachedEth() {\\n    \\_;\\n    uint256 remainingBalance =\\n        LibSafeMathV06.min256(msg.value, address(this).balance);\\n    if (remainingBalance > 0) {\\n        msg.sender.transfer(remainingBalance);\\n    }\\n}\\n```\\n\\nNotice that this modifier just returns the remaining ether balance (up to msg.value). It does not check for a specific amount of remaining ether. This meets condition (1) above.\\nIt's impossible to reenter the system with a second metatransaction because `executeMetaTransaction()` uses the modifier `nonReentrant`, but there's nothing preventing reentrancy via a different feature. We can achieve reentrancy by trading a token that uses callbacks (e.g. ERC777's hooks) during transfers. This meets condition (2).\\nTo find a full exploit, we also need a way to extract the ether held by the exchange proxy. `LiquidityProviderFeature.sellToLiquidityProvider()` provides such a mechanism. By passing `ETH_TOKEN_ADDRESS` as the `inputToken` and an address in the attacker's control as the `provider`, an attacker can transfer out any ether held by the exchange proxy. Note that `sellToLiquidityProvider()` can transfer any amount of ether, not limited to the amount sent via msg.value:\\n```\\nif (inputToken == ETH\\_TOKEN\\_ADDRESS) {\\n    provider.transfer(sellAmount);\\n```\\n\\nThis meets condition (3).\\nThe full steps to exploit this vulnerability are as follows:\\nA maker/attacker signs a trade where one of the tokens will invoke a callback during the trade.\\nA taker signs a metatransaction to take this trade.\\nA relayer sends in the metatransaction, providing more ether than is necessary to pay the protocol fee. (It's unclear how likely this situation is.)\\nDuring the token callback, the attacker invokes `LiquidityProviderFeature.sellToLiquidityProvider()` to transfer the excess ether to their account.\\nThe metatransaction feature returns the remaining ether balance, which is now zero.
In general, we recommend using strict accounting of ether throughout the system. If there's ever a temporary balance, it should be accurately resolved at the end of the transaction, after any potential reentrancy opportunities.\\nFor the example we specifically found, we recommend doing strict accounting in the metatransactions feature. This means features called via a metatransaction would need to return how much ether was consumed. The metatransactions feature could then refund exactly `msg.value - <consumed ether>`. The transaction should be reverted if this fails because it means ether went missing during the transaction.\\nWe also recommend limiting `sellToLiquidityProvider()` to only transfer up to `msg.value`. This is a form of defense in depth in case other vectors for a similar attack exist.
```\\n/// @dev Refunds up to `msg.value` leftover ETH at the end of the call.\\nmodifier refundsAttachedEth() {\\n    \\_;\\n    uint256 remainingBalance =\\n        LibSafeMathV06.min256(msg.value, address(this).balance);\\n    if (remainingBalance > 0) {\\n        msg.sender.transfer(remainingBalance);\\n    }\\n}\\n```\\n