Skip to content

Commit

Permalink
Avoid returndatacopy in ERC2771Forwarder by calling via assembly (#…
Browse files Browse the repository at this point in the history 
…4458)
  • Loading branch information
@ernestognw
ernestognw authored Jul 29, 2023
1 parent 02ea017 commit aed5720
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 17 deletions.
6 changes: 6 additions & 0 deletions contracts/metatx/ERC2771Context.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import {Context} from "../utils/Context.sol";

/**
* @dev Context variant with ERC2771 support.
*
* WARNING: Avoid using this pattern in contracts that rely in a specific calldata length as they'll
* be affected by any forwarder whose `msg.data` is suffixed with the `from` address according to the ERC2771
* specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
* behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
* function only accessible if `msg.data.length == 0`.
*/
abstract contract ERC2771Context is Context {
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
Expand Down
16 changes: 12 additions & 4 deletions contracts/metatx/ERC2771Forwarder.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,19 @@ contract ERC2771Forwarder is EIP712, Nonces {
// Nonce should be used before the call to prevent reusing by reentrancy
uint256 currentNonce = _useNonce(signer);

(success, ) = request.to.call{gas: request.gas, value: request.value}(
abi.encodePacked(request.data, request.from)
);
uint256 reqGas = request.gas;
address to = request.to;
uint256 value = request.value;
bytes memory data = abi.encodePacked(request.data, request.from);

_checkForwardedGas(gasleft(), request);
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);
}
Expand Down
112 changes: 99 additions & 13 deletions test/metatx/ERC2771Forwarder.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@ contract('ERC2771Forwarder', function (accounts) {
this.alice.address = web3.utils.toChecksumAddress(this.alice.getAddressString());

this.timestamp = await time.latest();
this.receiver = await CallReceiverMock.new();
this.request = {
from: this.alice.address,
to: constants.ZERO_ADDRESS,
to: this.receiver.address,
value: '0',
gas: '100000',
data: '0x',
data: this.receiver.contract.methods.mockFunction().encodeABI(),
deadline: this.timestamp.toNumber() + 60, // 1 minute
};
this.requestData = {
Expand All @@ -64,6 +65,14 @@ contract('ERC2771Forwarder', function (accounts) {
ethSigUtil.signTypedMessage(privateKey, {
data: this.forgeData(request),
});
this.estimateRequest = request =>
web3.eth.estimateGas({
from: this.forwarder.address,
to: request.to,
data: web3.utils.encodePacked({ value: request.data, type: 'bytes' }, { value: request.from, type: 'address' }),
value: request.value,
gas: request.gas,
});

this.requestData.signature = this.sign(this.alice.getPrivateKey());
});
Expand Down Expand Up @@ -125,7 +134,8 @@ contract('ERC2771Forwarder', function (accounts) {

it('emits an event and consumes nonce for a successful request', async function () {
const receipt = await this.forwarder.execute(this.requestData);
expectEvent(receipt, 'ExecutedForwardRequest', {
await expectEvent.inTransaction(receipt.tx, this.receiver, 'MockFunctionCalled');
await expectEvent.inTransaction(receipt.tx, this.forwarder, 'ExecutedForwardRequest', {
signer: this.requestData.from,
nonce: web3.utils.toBN(this.requestData.nonce),
success: true,
Expand All @@ -136,11 +146,9 @@ contract('ERC2771Forwarder', function (accounts) {
});

it('reverts with an unsuccessful request', async function () {
const receiver = await CallReceiverMock.new();
const req = {
...this.requestData,
to: receiver.address,
data: receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(),
data: this.receiver.contract.methods.mockFunctionRevertsNoReason().encodeABI(),
};
req.signature = this.sign(this.alice.getPrivateKey(), req);
await expectRevertCustomError(this.forwarder.execute(req), 'FailedInnerCall', []);
Expand Down Expand Up @@ -208,21 +216,44 @@ contract('ERC2771Forwarder', function (accounts) {
});

it('bubbles out of gas', async function () {
const receiver = await CallReceiverMock.new();
const gasAvailable = 100000;
this.requestData.to = receiver.address;
this.requestData.data = receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
this.requestData.gas = 1000000;

this.requestData.data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
this.requestData.gas = 1_000_000;
this.requestData.signature = this.sign(this.alice.getPrivateKey());

const gasAvailable = 100_000;
await expectRevert.assertion(this.forwarder.execute(this.requestData, { gas: gasAvailable }));

const { transactions } = await web3.eth.getBlock('latest');
const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);

expect(gasUsed).to.be.equal(gasAvailable);
});

it('bubbles out of gas forced by the relayer', async function () {
// If there's an incentive behind executing requests, a malicious relayer could grief
// the forwarder by executing requests and providing a top-level call gas limit that
// is too low to successfully finish the request after the 63/64 rule.

// We set the baseline to the gas limit consumed by a successful request if it was executed
// normally. Note this includes the 21000 buffer that also the relayer will be charged to
// start a request execution.
const estimate = await this.estimateRequest(this.request);

// Because the relayer call consumes gas until the `CALL` opcode, the gas left after failing
// the subcall won't enough to finish the top level call (after testing), so we add a
// moderated buffer.
const gasAvailable = estimate + 2_000;

// The subcall out of gas should be caught by the contract and then bubbled up consuming
// the available gas with an `invalid` opcode.
await expectRevert.outOfGas(this.forwarder.execute(this.requestData, { gas: gasAvailable }));

const { transactions } = await web3.eth.getBlock('latest');
const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);

// We assert that indeed the gas was totally consumed.
expect(gasUsed).to.be.equal(gasAvailable);
});
});

context('executeBatch', function () {
Expand Down Expand Up @@ -252,6 +283,14 @@ contract('ERC2771Forwarder', function (accounts) {
}));

this.msgValue = batchValue(this.requestDatas);

this.gasUntil = async reqIdx => {
const gas = 0;
const estimations = await Promise.all(
new Array(reqIdx + 1).fill().map((_, idx) => this.estimateRequest(this.requestDatas[idx])),
);
return estimations.reduce((acc, estimation) => acc + estimation, gas);
};
});

context('with valid requests', function () {
Expand All @@ -265,7 +304,8 @@ contract('ERC2771Forwarder', function (accounts) {

it('emits events', async function () {
for (const request of this.requestDatas) {
expectEvent(this.receipt, 'ExecutedForwardRequest', {
await expectEvent.inTransaction(this.receipt.tx, this.receiver, 'MockFunctionCalled');
await expectEvent.inTransaction(this.receipt.tx, this.forwarder, 'ExecutedForwardRequest', {
signer: request.from,
nonce: web3.utils.toBN(request.nonce),
success: true,
Expand Down Expand Up @@ -428,6 +468,52 @@ contract('ERC2771Forwarder', function (accounts) {
);
});
});

it('bubbles out of gas', async function () {
this.requestDatas[this.idx].data = this.receiver.contract.methods.mockFunctionOutOfGas().encodeABI();
this.requestDatas[this.idx].gas = 1_000_000;
this.requestDatas[this.idx].signature = this.sign(
this.signers[this.idx].getPrivateKey(),
this.requestDatas[this.idx],
);

const gasAvailable = 300_000;
await expectRevert.assertion(
this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, {
gas: gasAvailable,
value: this.requestDatas.reduce((acc, { value }) => acc + Number(value), 0),
}),
);

const { transactions } = await web3.eth.getBlock('latest');
const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);

expect(gasUsed).to.be.equal(gasAvailable);
});

it('bubbles out of gas forced by the relayer', async function () {
// Similarly to the single execute, a malicious relayer could grief requests.

// We estimate until the selected request as if they were executed normally
const estimate = await this.gasUntil(this.requestDatas, this.idx);

// We add a Buffer to account for all the gas that's used before the selected call.
// Note is slightly bigger because the selected request is not the index 0 and it affects
// the buffer needed.
const gasAvailable = estimate + 10_000;

// The subcall out of gas should be caught by the contract and then bubbled up consuming
// the available gas with an `invalid` opcode.
await expectRevert.outOfGas(
this.forwarder.executeBatch(this.requestDatas, constants.ZERO_ADDRESS, { gas: gasAvailable }),
);

const { transactions } = await web3.eth.getBlock('latest');
const { gasUsed } = await web3.eth.getTransactionReceipt(transactions[0]);

// We assert that indeed the gas was totally consumed.
expect(gasUsed).to.be.equal(gasAvailable);
});
});
});
});

0 comments on commit aed5720

Please sign in to comment.