Skip to content

Commit

Permalink
Merge pull request #40 from rainprotocol/2023-11-08-slither
Browse files Browse the repository at this point in the history 
wip on reentrant calculate orders
  • Loading branch information
@thedavidmeister
thedavidmeister authored Nov 20, 2023
2 parents 182c126 + f3541ea commit e794cc9
Show file tree
Hide file tree
Showing 22 changed files with 1,160 additions and 466 deletions.
144 changes: 87 additions & 57 deletions .gas-snapshot
View file

Large diffs are not rendered by default.

Binary file modified meta/OrderBook.rain.meta
View file
Binary file not shown.
2 changes: 1 addition & 1 deletion slither.config.json
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"detectors_to_exclude": "assembly-usage",
"detectors_to_exclude": "assembly-usage,solc-version",
"filter_paths": "forge-std,openzeppelin,test"
}
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
171 changes: 21 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,38 @@ 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();
// This behaviour is copied almost verbatim from the ERC3156 spec.
// Slither is complaining because this kind of logic can normally be used
// to grief the token holder. Consider if alice were to approve order book
// for the sake of depositing and then bob could cause alice to send
// tokens to order book without their consent. However, in this case the
// flash loan spec provides two reasons that this is not a problem:
// - We just sent this exact amount to the receiver as the loan, so
// transferring them back with a 0 fee is net neutral.
// - The receiver is a contract that has explicitly opted in to this
// behaviour by implementing `IERC3156FlashBorrower`. The success check
// for `onFlashLoan` guarantees the receiver has opted into this
// behaviour independently of any approvals, etc.
// https://github.com/crytic/slither/issues/1658
//slither-disable-next-line arbitrary-send-erc20
IERC20(token).safeTransferFrom(address(receiver), address(this), amount + FLASH_FEE);

return true;
}
Expand All @@ -195,6 +66,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));
}
}
Loading

0 comments on commit e794cc9

Please sign in to comment.