Skip to content

Commit

Permalink
wip on slither
Browse files Browse the repository at this point in the history
  • Loading branch information
@thedavidmeister
thedavidmeister committed Nov 8, 2023
1 parent 7cffaf5 commit 91cdd60
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 191 deletions.
2 changes: 1 addition & 1 deletion src/abstract/OrderBookV3ArbCommon.sol
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: CAL
pragma solidity ^0.8.18;
pragma solidity ^0.8.19;

/// Thrown when the minimum output for the sender is not met after the arb.
/// @param minimum The minimum output expected by the sender.
Expand Down
2 changes: 1 addition & 1 deletion src/abstract/OrderBookV3ArbOrderTaker.sol
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: CAL
pragma solidity =0.8.19;
pragma solidity ^0.8.19;

import {ERC165, IERC165} from "lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol";
import {ReentrancyGuard} from "lib/openzeppelin-contracts/contracts/security/ReentrancyGuard.sol";
Expand Down
23 changes: 15 additions & 8 deletions src/abstract/OrderBookV3FlashBorrower.sol
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: CAL
pragma solidity =0.8.19;
pragma solidity ^0.8.19;

import {ERC165, IERC165} from "lib/openzeppelin-contracts/contracts/utils/introspection/ERC165.sol";
import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
Expand All @@ -12,13 +12,20 @@ import {
DeployerDiscoverableMetaV2ConstructionConfig,
LibMeta
} from "lib/rain.interpreter/src/abstract/DeployerDiscoverableMetaV2.sol";
import "lib/rain.interpreter/src/lib/caller/LibEncodedDispatch.sol";
import "lib/rain.interpreter/src/lib/caller/LibContext.sol";
import "lib/rain.interpreter/src/lib/bytecode/LibBytecode.sol";

import "../interface/unstable/IOrderBookV3.sol";
import "lib/rain.factory/src/interface/ICloneableV2.sol";
import "./OrderBookV3ArbCommon.sol";
import {LibEncodedDispatch, EncodedDispatch} from "lib/rain.interpreter/src/lib/caller/LibEncodedDispatch.sol";
import {LibContext} from "lib/rain.interpreter/src/lib/caller/LibContext.sol";
import {LibBytecode} from "lib/rain.interpreter/src/lib/bytecode/LibBytecode.sol";
import {ON_FLASH_LOAN_CALLBACK_SUCCESS} from "../interface/ierc3156/IERC3156FlashBorrower.sol";
import {EvaluableConfigV2} from "rain.interpreter/src/lib/caller/LibEvaluable.sol";
import {IOrderBookV3, TakeOrdersConfigV2, NoOrders} from "../interface/unstable/IOrderBookV3.sol";
import {ICloneableV2, ICLONEABLE_V2_SUCCESS} from "lib/rain.factory/src/interface/ICloneableV2.sol";
import {
IInterpreterV1, SourceIndex, DEFAULT_STATE_NAMESPACE
} from "lib/rain.interpreter/src/interface/IInterpreterV1.sol";
import {IERC3156FlashBorrower} from "../interface/ierc3156/IERC3156FlashBorrower.sol";
import {IInterpreterStoreV1} from "lib/rain.interpreter/src/interface/IInterpreterStoreV1.sol";
import {BadLender, MinimumOutput, NonZeroBeforeArbStack, Initializing} from "./OrderBookV3ArbCommon.sol";
import {SignedContextV1} from "rain.interpreter/src/interface/IInterpreterCallerV2.sol";

/// Thrown when the initiator is not the order book.
/// @param badInitiator The untrusted initiator of the flash loan.
Expand Down
157 changes: 7 additions & 150 deletions src/abstract/OrderBookV3FlashLender.sol
View file
Original file line number Diff line number Diff line change
@@ -1,33 +1,17 @@
// SPDX-License-Identifier: CAL
pragma solidity ^0.8.18;
pragma solidity ^0.8.19;

import {Math} from "lib/openzeppelin-contracts/contracts/utils/math/Math.sol";
import {IERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";

import "../interface/ierc3156/IERC3156FlashBorrower.sol";
import "../interface/ierc3156/IERC3156FlashLender.sol";

/// Thrown when `flashLoan` token is zero address.
error ZeroToken();

/// Thrown when `flashLoan` receiver is zero address.
error ZeroReceiver();

/// Thrown when `flashLoan` amount is zero.
error ZeroAmount();
import {IERC3156FlashBorrower, ON_FLASH_LOAN_CALLBACK_SUCCESS} from "../interface/ierc3156/IERC3156FlashBorrower.sol";
import {IERC3156FlashLender} from "../interface/ierc3156/IERC3156FlashLender.sol";

/// Thrown when the `onFlashLoan` callback returns anything other than
/// ON_FLASH_LOAN_CALLBACK_SUCCESS.
/// @param result The value that was returned by `onFlashLoan`.
error FlashLenderCallbackFailed(bytes32 result);

/// Thrown when more than one debt is attempted simultaneously.
/// @param receiver The receiver of the active debt.
/// @param token The token of the active debt.
/// @param amount The amount of the active debt.
error ActiveDebt(address receiver, address token, uint256 amount);

/// @dev Flash fee is always 0 for orderbook as there's no entity to take
/// revenue for `Orderbook` and its more important anyway that flashloans happen
/// to connect external liquidity to live orders via arbitrage.
Expand All @@ -36,151 +20,24 @@ uint256 constant FLASH_FEE = 0;
/// @title OrderBookV3FlashLender
/// @notice Implements `IERC3156FlashLender` for `OrderBook`. Based on the
/// reference implementation by Alberto Cuesta Cañada found at
/// https://eips.ethereum.org/EIPS/eip-3156
/// Several features found in the reference implementation are simplified or
/// hardcoded for `OrderBookV3`.
/// https://eips.ethereum.org/EIPS/eip-3156#flash-loan-reference-implementation
abstract contract OrderBookV3FlashLender is IERC3156FlashLender {
using Math for uint256;
using SafeERC20 for IERC20;

IERC3156FlashBorrower private _sReceiver = IERC3156FlashBorrower(address(0));
address private _sToken = address(0);
uint256 private _sAmountUnpaid = 0;

/// If any of the debt related storage values are set then we consider a
/// debt active. Notably the debt is still active even if the amount unpaid
/// is zero, until the loan originating `flashLoan` call fully completes.
function _isActiveDebt() internal view returns (bool) {
return (address(_sReceiver) != address(0)) || (_sToken != address(0)) || (_sAmountUnpaid != 0);
}

function _checkActiveDebt() internal view {
if (_isActiveDebt()) {
revert ActiveDebt(address(_sReceiver), _sToken, _sAmountUnpaid);
}
}

/// Whenever `Orderbook` sends tokens to any address it MUST first attempt
/// to decrease any outstanding flash loans for that address. Consider the
/// case that Alice deposits 100 TKN and she is the only depositor of TKN
/// then flash borrows 100 TKN. If she attempts to withdraw 100 TKN during
/// her `onFlashLoan` callback then `Orderbook`:
///
/// - has 0 TKN balance to process the withdrawal
/// - MUST process the withdrawal as Alice has the right to withdraw her
/// balance at any time
/// - Has the 100 TKN debt active under Alice
///
/// In this case `Orderbook` can simply forgive Alice's 100 TKN debt instead
/// of actually transferring any tokens. The withdrawal can decrease her
/// vault balance by 100 TKN decoupled from needing to know whether a
/// tranfer or forgiveness happened.
///
/// The same logic applies to withdrawals as sending tokens during
/// `takeOrders` as the reason for sending tokens is irrelevant, all that
/// matters is that `Orderbook` prioritises debt repayments over external
/// transfers.
///
/// If there is an active debt that only partially eclipses the withdrawal
/// then the debt will be fully repaid and the remainder transferred as a
/// real token transfer.
///
/// Note that Alice can still contrive a situation that causes `Orderbook`
/// to attempt to send tokens that it does not have. If Alice can write a
/// smart contract to trigger withdrawals she can flash loan 100% of the
/// TKN supply in `Orderbook` and trigger her contract to attempt a
/// withdrawal. For any normal ERC20 token this will fail and revert as the
/// `Orderbook` cannot send tokens it does not have under any circumstances,
/// but the scenario is worth being aware of for more exotic token
/// behaviours that may not be supported.
///
/// @param token The token being sent or for the debt being paid.
/// @param receiver The receiver of the token or holder of the debt.
/// @param sendAmount The amount to send or repay.
/// @return The final amount sent after any debt repayment.
function _decreaseFlashDebtThenSendToken(address token, address receiver, uint256 sendAmount)
internal
returns (uint256)
{
// If this token transfer matches the active debt then prioritise
// reducing debt over sending tokens.
if (token == _sToken && receiver == address(_sReceiver)) {
uint256 debtReduction = sendAmount.min(_sAmountUnpaid);
sendAmount -= debtReduction;

// Even if this completely zeros the amount the debt is considered
// active until the `flashLoan` also clears the token and recipient.
_sAmountUnpaid -= debtReduction;
}

if (sendAmount > 0) {
IERC20(token).safeTransfer(receiver, sendAmount);
}
return sendAmount;
}

/// @inheritdoc IERC3156FlashLender
function flashLoan(IERC3156FlashBorrower receiver, address token, uint256 amount, bytes calldata data)
external
override
returns (bool)
{
// Set the active debt before transferring tokens to prevent reeentrancy.
// The active debt is set beyond the scope of `flashLoan` to facilitate
// early repayment via. `_decreaseFlashDebtThenSendToken`.
{
if (address(receiver) == address(0)) {
revert ZeroReceiver();
}
if (token == address(0)) {
revert ZeroToken();
}
if (amount == 0) {
revert ZeroAmount();
}

// This prevents reentrancy, loans can be taken sequentially within
// a transaction but not simultanously. If any of these values are
// nonzero in storage, they cannot be set to a new value.
_checkActiveDebt();
_sToken = token;
_sReceiver = receiver;
// As long as FLASH_FEE is 0 this `+` will probably get optimised
// away by the compiler.
_sAmountUnpaid = amount + FLASH_FEE;
IERC20(token).safeTransfer(address(receiver), amount);
}
IERC20(token).safeTransfer(address(receiver), amount);

bytes32 result = receiver.onFlashLoan(msg.sender, token, amount, FLASH_FEE, data);
if (result != ON_FLASH_LOAN_CALLBACK_SUCCESS) {
revert FlashLenderCallbackFailed(result);
}

// Pull tokens before releasing the active debt to prevent a new loan
// from being taken reentrantly during the repayment of the current loan.
{
// Sync local `amount_` with global `_amount` in case an early
// repayment was made during the loan term via.
// `_decreaseFlashDebtThenSendToken`.
amount = _sAmountUnpaid;
if (amount > 0) {
// There is no way to fix this slither warning and be compatible
// with ERC3156.
// https://github.com/crytic/slither/issues/1658
//slither-disable-next-line arbitrary-send-erc20
IERC20(token).safeTransferFrom(address(receiver), address(this), amount);
_sAmountUnpaid = 0;
}

// Both of these are required to fully clear the active debt and
// allow new debts.
_sReceiver = IERC3156FlashBorrower(address(0));
_sToken = address(0);
}

// Guard against some bad code path that allowed an active debt to remain
// at this point. Should be impossible.
_checkActiveDebt();
IERC20(token).safeTransferFrom(address(receiver), address(this), amount + FLASH_FEE);

return true;
}
Expand All @@ -195,6 +52,6 @@ abstract contract OrderBookV3FlashLender is IERC3156FlashLender {
/// then loans are disabled so the max becomes `0` until after repayment.
/// @inheritdoc IERC3156FlashLender
function maxFlashLoan(address token) external view override returns (uint256) {
return _isActiveDebt() ? 0 : IERC20(token).balanceOf(address(this));
return IERC20(token).balanceOf(address(this));
}
}
42 changes: 12 additions & 30 deletions src/concrete/OrderBook.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,32 +207,17 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash
DeployerDiscoverableMetaV2(CALLER_META_HASH, config)
{}

/// Guard against read-only reentrancy.
/// https://chainsecurity.com/heartbreaks-curve-lp-oracles/
modifier nonReentrantView() {
if (_reentrancyGuardEntered()) {
revert ReentrancyGuardReentrantCall();
}
_;
}

/// @inheritdoc IOrderBookV3
// This has a reentrancy guard on it but Slither doesn't know that because
// it is a read-only reentrancy due to potential cross function reentrancy.
// https://github.com/crytic/slither/issues/735#issuecomment-1620704314
//slither-disable-next-line reentrancy-no-eth
function vaultBalance(address owner, address token, uint256 vaultId)
external
view
override
nonReentrantView
returns (uint256)
{
function vaultBalance(address owner, address token, uint256 vaultId) external view override returns (uint256) {
return sVaultBalances[owner][token][vaultId];
}

/// @inheritdoc IOrderBookV3
function orderExists(bytes32 orderHash) external view override nonReentrantView returns (bool) {
function orderExists(bytes32 orderHash) external view override returns (bool) {
return sOrders[orderHash] == ORDER_LIVE;
}

Expand Down Expand Up @@ -264,7 +249,7 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash
// withdrawals to exceed vault balances.
sVaultBalances[msg.sender][token][vaultId] = currentVaultBalance - withdrawAmount;
emit Withdraw(msg.sender, token, vaultId, targetAmount, withdrawAmount);
_decreaseFlashDebtThenSendToken(token, msg.sender, withdrawAmount);
IERC20(token).safeTransfer(msg.sender, withdrawAmount);
}
}

Expand Down Expand Up @@ -495,34 +480,31 @@ contract OrderBook is IOrderBookV3, ReentrancyGuard, Multicall, OrderBookV3Flash
revert MinimumInput(config.minimumInput, totalTakerInput);
}

// Prioritise paying down any active flash loans before sending any
// tokens to `msg.sender`. We send the tokens to `msg.sender` first
// adopting a similar pattern to Uniswap flash swaps. We call the caller
// before attempting to pull tokens from them in order to facilitate
// better integrations with external liquidity sources. This could be
// done by the caller using flash loans but this callback:
// We send the tokens to `msg.sender` first adopting a similar pattern to
// Uniswap flash swaps. We call the caller before attempting to pull
// tokens from them in order to facilitate better integrations with
// external liquidity sources. This could be done by the caller using
// flash loans but this callback:
// - may be simpler for the caller to implement
// - allows the caller to call `takeOrders` _before_ placing external
// trades, which is important if the order logic itself is dependent on
// external data (e.g. prices) that could be modified by the caller's
// trades.
uint256 takerInputAmountSent = _decreaseFlashDebtThenSendToken(
config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token, msg.sender, totalTakerInput

IERC20(config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token).safeTransfer(
msg.sender, totalTakerInput
);
if (config.data.length > 0) {
IOrderBookV3OrderTaker(msg.sender).onTakeOrders(
config.orders[0].order.validOutputs[config.orders[0].outputIOIndex].token,
config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token,
takerInputAmountSent,
totalTakerInput,
totalTakerOutput,
config.data
);
}

if (totalTakerOutput > 0) {
// We already updated vault balances before we took tokens from
// `msg.sender` which is usually NOT the correct order of operations for
// depositing to a vault. We rely on reentrancy guards to make this safe.
IERC20(config.orders[0].order.validInputs[config.orders[0].inputIOIndex].token).safeTransferFrom(
msg.sender, address(this), totalTakerOutput
);
Expand Down
2 changes: 1 addition & 1 deletion src/interface/ierc3156/IERC3156FlashLender.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Alberto Cuesta Cañada, Fiona Kobayashi, fubuloubu, Austin Williams, "EIP-3156: Flash Loans," Ethereum Improvement Proposals, no. 3156, November 2020. [Online serial]. Available: https://eips.ethereum.org/EIPS/eip-3156.
pragma solidity ^0.8.18;

import "./IERC3156FlashBorrower.sol";
import {IERC3156FlashBorrower} from "./IERC3156FlashBorrower.sol";

interface IERC3156FlashLender {
/**
Expand Down

0 comments on commit 91cdd60

Please sign in to comment.