github/scroll
1
1
Fork 0
mirror of https://github.com/scroll-tech/scroll.git synced 2026-01-10 22:48:14 -05:00
Code Issues Packages Projects Releases Wiki Activity

fix(contracts): OZ-M04 Use of Non-Production-Ready Trusted Forwarder (#843)

Browse Source 
Co-authored-by: Haichen Shen <shenhaichen@gmail.com>
Co-authored-by: icemelon <icemelon@users.noreply.github.com>
This commit is contained in:
Xi Lin
2023-09-02 17:23:31 +08:00
committed by GitHub
parent d3c2e34650
commit d668180e9a
4 changed files with 466 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
common/version/version.go
View File

@@ -5,7 +5,7 @@ import (
"runtime/debug"
)
var tag = "v4.2.17"
var tag = "v4.2.18"
var commit = func() string {
if info, ok := debug.ReadBuildInfo(); ok {

33
contracts/integration-test/GasSwap.spec.ts
View File

@@ -1,7 +1,7 @@
/* eslint-disable node/no-unpublished-import */
/* eslint-disable node/no-missing-import */
import { ethers } from "hardhat";
import { GasSwap, MinimalForwarder, MockERC20, MockGasSwapTarget } from "../typechain";
import { GasSwap, ERC2771Forwarder, MockERC20, MockGasSwapTarget } from "../typechain";
import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers";
import { expect } from "chai";
import { BigNumber, constants } from "ethers";
@@ -11,7 +11,7 @@ describe("GasSwap.spec", async () => {
let deployer: SignerWithAddress;
let signer: SignerWithAddress;
let forwarder: MinimalForwarder;
let forwarder: ERC2771Forwarder;
let swap: GasSwap;
let target: MockGasSwapTarget;
let token: MockERC20;
@@ -19,8 +19,8 @@ describe("GasSwap.spec", async () => {
beforeEach(async () => {
[deployer, signer] = await ethers.getSigners();
const MinimalForwarder = await ethers.getContractFactory("MinimalForwarder", deployer);
forwarder = await MinimalForwarder.deploy();
const ERC2771Forwarder = await ethers.getContractFactory("ERC2771Forwarder", deployer);
forwarder = await ERC2771Forwarder.deploy("ERC2771Forwarder");
await forwarder.deployed();
const GasSwap = await ethers.getContractFactory("GasSwap", deployer);
@@ -253,12 +253,13 @@ describe("GasSwap.spec", async () => {
await swap.updateFeeRatio(ethers.utils.parseEther(feeRatio).div(100));
const fee = amountOut.mul(feeRatio).div(100);
const req = {
const reqWithoutSignature = {
from: signer.address,
to: swap.address,
value: constants.Zero,
gas: 1000000,
nonce: 0,
nonce: await forwarder.nonces(signer.address),
deadline: 2000000000,
data: swap.interface.encodeFunctionData("swap", [
{
token: token.address,
@@ -278,8 +279,8 @@ describe("GasSwap.spec", async () => {
const signature = await signer._signTypedData(
{
name: "MinimalForwarder",
version: "0.0.1",
name: "ERC2771Forwarder",
version: "1",
chainId: (await ethers.provider.getNetwork()).chainId,
verifyingContract: forwarder.address,
},
@@ -305,17 +306,29 @@ describe("GasSwap.spec", async () => {
name: "nonce",
type: "uint256",
},
{
name: "deadline",
type: "uint48",
},
{
name: "data",
type: "bytes",
},
],
},
req
reqWithoutSignature
);
const balanceBefore = await signer.getBalance();
await forwarder.execute(req, signature);
await forwarder.execute({
from: reqWithoutSignature.from,
to: reqWithoutSignature.to,
value: reqWithoutSignature.value,
gas: reqWithoutSignature.gas,
deadline: reqWithoutSignature.deadline,
data: reqWithoutSignature.data,
signature,
});
const balanceAfter = await signer.getBalance();
expect(balanceAfter.sub(balanceBefore)).to.eq(amountOut.sub(fee));
expect(await token.balanceOf(signer.address)).to.eq(amountIn.mul(refundRatio).div(100));

391
contracts/src/misc/ERC2771Forwarder.sol Normal file
View File

@@ -0,0 +1,391 @@
// SPDX-License-Identifier: MIT
// @note This file is directly copied from OpenZeppelin's master branch:
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/metatx/ERC2771Forwarder.sol
// Modifications are made to make it compatible with solidity 0.8.16.
pragma solidity =0.8.16;
import {ERC2771Context} from "@openzeppelin/contracts/metatx/ERC2771Context.sol";
import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import {EIP712} from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
import {Nonces} from "./Nonces.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
/**
* @dev A forwarder compatible with ERC2771 contracts. See {ERC2771Context}.
*
* This forwarder operates on forward requests that include:
*
* * `from`: An address to operate on behalf of. It is required to be equal to the request signer.
* * `to`: The address that should be called.
* * `value`: The amount of native token to attach with the requested call.
* * `gas`: The amount of gas limit that will be forwarded with the requested call.
* * `nonce`: A unique transaction ordering identifier to avoid replayability and request invalidation.
* * `deadline`: A timestamp after which the request is not executable anymore.
* * `data`: Encoded `msg.data` to send with the requested call.
*
* Relayers are able to submit batches if they are processing a high volume of requests. With high
* throughput, relayers may run into limitations of the chain such as limits on the number of
* transactions in the mempool. In these cases the recommendation is to distribute the load among
* multiple accounts.
*
* NOTE: Batching requests includes an optional refund for unused `msg.value` that is achieved by
* performing a call with empty calldata. While this is within the bounds of ERC-2771 compliance,
* if the refund receiver happens to consider the forwarder a trusted forwarder, it MUST properly
* handle `msg.data.length == 0`. `ERC2771Context` in OpenZeppelin Contracts versions prior to 4.9.3
* do not handle this properly.
*
* ==== Security Considerations
*
* If a relayer submits a forward request, it should be willing to pay up to 100% of the gas amount
* specified in the request. This contract does not implement any kind of retribution for this gas,
* and it is assumed that there is an out of band incentive for relayers to pay for execution on
* behalf of signers. Often, the relayer is operated by a project that will consider it a user
* acquisition cost.
*
* By offering to pay for gas, relayers are at risk of having that gas used by an attacker toward
* some other purpose that is not aligned with the expected out of band incentives. If you operate a
* relayer, consider whitelisting target contracts and function selectors. When relaying ERC-721 or
* ERC-1155 transfers specifically, consider rejecting the use of the `data` field, since it can be
* used to execute arbitrary code.
*/
contract ERC2771Forwarder is EIP712, Nonces {
using ECDSA for bytes32;
struct ForwardRequestData {
address from;
address to;
uint256 value;
uint256 gas;
uint48 deadline;
bytes data;
bytes signature;
}
bytes32 internal constant _FORWARD_REQUEST_TYPEHASH =
keccak256(
"ForwardRequest(address from,address to,uint256 value,uint256 gas,uint256 nonce,uint48 deadline,bytes data)"
);
/**
* @dev Emitted when a `ForwardRequest` is executed.
*
* NOTE: An unsuccessful forward request could be due to an invalid signature, an expired deadline,
* or simply a revert in the requested call. The contract guarantees that the relayer is not able to force
* the requested call to run out of gas.
*/
event ExecutedForwardRequest(address indexed signer, uint256 nonce, bool success);
/**
* @dev The request `from` doesn't match with the recovered `signer`.
*/
error ERC2771ForwarderInvalidSigner(address signer, address from);
/**
* @dev The `requestedValue` doesn't match with the available `msgValue`.
*/
error ERC2771ForwarderMismatchedValue(uint256 requestedValue, uint256 msgValue);
/**
* @dev The request `deadline` has expired.
*/
error ERC2771ForwarderExpiredRequest(uint48 deadline);
/**
* @dev The request target doesn't trust the `forwarder`.
*/
error ERC2771UntrustfulTarget(address target, address forwarder);
/**
* @dev A call to an address target failed. The target may have reverted.
*/
error FailedInnerCall();
/**
* @dev See {EIP712-constructor}.
*/
constructor(string memory name) EIP712(name, "1") {}
/**
* @dev Returns `true` if a request is valid for a provided `signature` at the current block timestamp.
*
* A transaction is considered valid when the target trusts this forwarder, the request hasn't expired
* (deadline is not met), and the signer matches the `from` parameter of the signed request.
*
* NOTE: A request may return false here but it won't cause {executeBatch} to revert if a refund
* receiver is provided.
*/
function verify(ForwardRequestData calldata request) public view virtual returns (bool) {
(bool isTrustedForwarder, bool active, bool signerMatch, ) = _validate(request);
return isTrustedForwarder && active && signerMatch;
}
/**
* @dev Executes a `request` on behalf of `signature`'s signer using the ERC-2771 protocol. The gas
* provided to the requested call may not be exactly the amount requested, but the call will not run
* out of gas. Will revert if the request is invalid or the call reverts, in this case the nonce is not consumed.
*
* Requirements:
*
* - The request value should be equal to the provided `msg.value`.
* - The request should be valid according to {verify}.
*/
function execute(ForwardRequestData calldata request) public payable virtual {
// We make sure that msg.value and request.value match exactly.
// If the request is invalid or the call reverts, this whole function
// will revert, ensuring value isn't stuck.
if (msg.value != request.value) {
revert ERC2771ForwarderMismatchedValue(request.value, msg.value);
}
if (!_execute(request, true)) {
revert FailedInnerCall();
}
}
/**
* @dev Batch version of {execute} with optional refunding and atomic execution.
*
* In case a batch contains at least one invalid request (see {verify}), the
* request will be skipped and the `refundReceiver` parameter will receive back the
* unused requested value at the end of the execution. This is done to prevent reverting
* the entire batch when a request is invalid or has already been submitted.
*
* If the `refundReceiver` is the `address(0)`, this function will revert when at least
* one of the requests was not valid instead of skipping it. This could be useful if
* a batch is required to get executed atomically (at least at the top-level). For example,
* refunding (and thus atomicity) can be opt-out if the relayer is using a service that avoids
* including reverted transactions.
*
* Requirements:
*
* - The sum of the requests' values should be equal to the provided `msg.value`.
* - All of the requests should be valid (see {verify}) when `refundReceiver` is the zero address.
*
* NOTE: Setting a zero `refundReceiver` guarantees an all-or-nothing requests execution only for
* the first-level forwarded calls. In case a forwarded request calls to a contract with another
* subcall, the second-level call may revert without the top-level call reverting.
*/
function executeBatch(ForwardRequestData[] calldata requests, address payable refundReceiver)
public
payable
virtual
{
bool atomic = refundReceiver == address(0);
uint256 requestsValue;
uint256 refundValue;
for (uint256 i; i < requests.length; ++i) {
requestsValue += requests[i].value;
bool success = _execute(requests[i], atomic);
if (!success) {
refundValue += requests[i].value;
}
}
// The batch should revert if there's a mismatched msg.value provided
// to avoid request value tampering
if (requestsValue != msg.value) {
revert ERC2771ForwarderMismatchedValue(requestsValue, msg.value);
}
// Some requests with value were invalid (possibly due to frontrunning).
// To avoid leaving ETH in the contract this value is refunded.
if (refundValue != 0) {
// We know refundReceiver != address(0) && requestsValue == msg.value
// meaning we can ensure refundValue is not taken from the original contract's balance
// and refundReceiver is a known account.
Address.sendValue(refundReceiver, refundValue);
}
}
/**
* @dev Validates if the provided request can be executed at current block timestamp with
* the given `request.signature` on behalf of `request.signer`.
*/
function _validate(ForwardRequestData calldata request)
internal
view
virtual
returns (
bool isTrustedForwarder,
bool active,
bool signerMatch,
address signer
)
{
(bool isValid, address recovered) = _recoverForwardRequestSigner(request);
return (
_isTrustedByTarget(request.to),
request.deadline >= block.timestamp,
isValid && recovered == request.from,
recovered
);
}
/**
* @dev Returns a tuple with the recovered the signer of an EIP712 forward request message hash
* and a boolean indicating if the signature is valid.
*
* NOTE: The signature is considered valid if {ECDSA-tryRecover} indicates no recover error for it.
*/
function _recoverForwardRequestSigner(ForwardRequestData calldata request)
internal
view
virtual
returns (bool, address)
{
(address recovered, ECDSA.RecoverError err) = _hashTypedDataV4(
keccak256(
abi.encode(
_FORWARD_REQUEST_TYPEHASH,
request.from,
request.to,
request.value,
request.gas,
nonces(request.from),
request.deadline,
keccak256(request.data)
)
)
).tryRecover(request.signature);
return (err == ECDSA.RecoverError.NoError, recovered);
}
/**
* @dev Validates and executes a signed request returning the request call `success` value.
*
* Internal function without msg.value validation.
*
* Requirements:
*
* - The caller must have provided enough gas to forward with the call.
* - The request must be valid (see {verify}) if the `requireValidRequest` is true.
*
* Emits an {ExecutedForwardRequest} event.
*
* IMPORTANT: Using this function doesn't check that all the `msg.value` was sent, potentially
* leaving value stuck in the contract.
*/
function _execute(ForwardRequestData calldata request, bool requireValidRequest)
internal
virtual
returns (bool success)
{
(bool isTrustedForwarder, bool active, bool signerMatch, address signer) = _validate(request);
// Need to explicitly specify if a revert is required since non-reverting is default for
// batches and reversion is opt-in since it could be useful in some scenarios
if (requireValidRequest) {
if (!isTrustedForwarder) {
revert ERC2771UntrustfulTarget(request.to, address(this));
}
if (!active) {
revert ERC2771ForwarderExpiredRequest(request.deadline);
}
if (!signerMatch) {
revert ERC2771ForwarderInvalidSigner(signer, request.from);
}
}
// Ignore an invalid request because requireValidRequest = false
if (isTrustedForwarder && signerMatch && active) {
// Nonce should be used before the call to prevent reusing by reentrancy
uint256 currentNonce = _useNonce(signer);
uint256 reqGas = request.gas;
address to = request.to;
uint256 value = request.value;
bytes memory data = abi.encodePacked(request.data, request.from);
uint256 gasLeft;
assembly {
success := call(reqGas, to, value, add(data, 0x20), mload(data), 0, 0)
gasLeft := gas()
}
_checkForwardedGas(gasLeft, request);
emit ExecutedForwardRequest(signer, currentNonce, success);
}
}
/**
* @dev Returns whether the target trusts this forwarder.
*
* This function performs a static call to the target contract calling the
* {ERC2771Context-isTrustedForwarder} function.
*/
function _isTrustedByTarget(address target) private view returns (bool) {
bytes memory encodedParams = abi.encodeCall(ERC2771Context.isTrustedForwarder, (address(this)));
bool success;
uint256 returnSize;
uint256 returnValue;
/// @solidity memory-safe-assembly
assembly {
// Perform the staticcal and save the result in the scratch space.
// | Location | Content | Content (Hex) |
// |-----------|----------|--------------------------------------------------------------------|
// | | | result ↓ |
// | 0x00:0x1F | selector | 0x0000000000000000000000000000000000000000000000000000000000000001 |
success := staticcall(gas(), target, add(encodedParams, 0x20), mload(encodedParams), 0, 0x20)
returnSize := returndatasize()
returnValue := mload(0)
}
return success && returnSize >= 0x20 && returnValue > 0;
}
/**
* @dev Checks if the requested gas was correctly forwarded to the callee.
*
* As a consequence of https://eips.ethereum.org/EIPS/eip-150[EIP-150]:
* - At most `gasleft() - floor(gasleft() / 64)` is forwarded to the callee.
* - At least `floor(gasleft() / 64)` is kept in the caller.
*
* It reverts consuming all the available gas if the forwarded gas is not the requested gas.
*
* IMPORTANT: The `gasLeft` parameter should be measured exactly at the end of the forwarded call.
* Any gas consumed in between will make room for bypassing this check.
*/
function _checkForwardedGas(uint256 gasLeft, ForwardRequestData calldata request) private pure {
// To avoid insufficient gas griefing attacks, as referenced in https://ronan.eth.limo/blog/ethereum-gas-dangers/
//
// A malicious relayer can attempt to shrink the gas forwarded so that the underlying call reverts out-of-gas
// but the forwarding itself still succeeds. In order to make sure that the subcall received sufficient gas,
// we will inspect gasleft() after the forwarding.
//
// Let X be the gas available before the subcall, such that the subcall gets at most X * 63 / 64.
// We can't know X after CALL dynamic costs, but we want it to be such that X * 63 / 64 >= req.gas.
// Let Y be the gas used in the subcall. gasleft() measured immediately after the subcall will be gasleft() = X - Y.
// If the subcall ran out of gas, then Y = X * 63 / 64 and gasleft() = X - Y = X / 64.
// Under this assumption req.gas / 63 > gasleft() is true is true if and only if
// req.gas / 63 > X / 64, or equivalently req.gas > X * 63 / 64.
// This means that if the subcall runs out of gas we are able to detect that insufficient gas was passed.
//
// We will now also see that req.gas / 63 > gasleft() implies that req.gas >= X * 63 / 64.
// The contract guarantees Y <= req.gas, thus gasleft() = X - Y >= X - req.gas.
// - req.gas / 63 > gasleft()
// - req.gas / 63 >= X - req.gas
// - req.gas >= X * 63 / 64
// In other words if req.gas < X * 63 / 64 then req.gas / 63 <= gasleft(), thus if the relayer behaves honestly
// the forwarding does not revert.
if (gasLeft < request.gas / 63) {
// We explicitly trigger invalid opcode to consume all gas and bubble-up the effects, since
// neither revert or assert consume all gas since Solidity 0.8.20
// https://docs.soliditylang.org/en/v0.8.20/control-structures.html#panic-via-assert-and-error-via-require
/// @solidity memory-safe-assembly
assembly {
invalid()
}
}
}
}

51
contracts/src/misc/Nonces.sol Normal file
View File

@@ -0,0 +1,51 @@
// SPDX-License-Identifier: MIT
// @note This file is directly copied from OpenZeppelin's master branch:
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Nonces.sol
// Modifications are made to make it compatible with solidity 0.8.16.
pragma solidity ^0.8.16;
/**
* @dev Provides tracking nonces for addresses. Nonces will only increment.
*/
abstract contract Nonces {
/**
* @dev The nonce used for an `account` is not the expected current nonce.
*/
error InvalidAccountNonce(address account, uint256 currentNonce);
mapping(address => uint256) private _nonces;
/**
* @dev Returns an the next unused nonce for an address.
*/
function nonces(address owner) public view virtual returns (uint256) {
return _nonces[owner];
}
/**
* @dev Consumes a nonce.
*
* Returns the current value and increments nonce.
*/
function _useNonce(address owner) internal virtual returns (uint256) {
// For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be
// decremented or reset. This guarantees that the nonce never overflows.
unchecked {
// It is important to do x++ and not ++x here.
return _nonces[owner]++;
}
}
/**
* @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`.
*/
function _useCheckedNonce(address owner, uint256 nonce) internal virtual returns (uint256) {
uint256 current = _useNonce(owner);
if (nonce != current) {
revert InvalidAccountNonce(owner, current);
}
return current;
}
}