From ccfffe13e815f2e8eba20d3ab16d568557e97dd6 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 7 Dec 2023 15:30:08 -0600 Subject: [PATCH] Make Multicall context-aware --- .changeset/rude-weeks-beg.md | 5 +++ .changeset/strong-points-invent.md | 5 +++ contracts/metatx/ERC2771Context.sol | 29 +++++++++----- contracts/mocks/ERC2771ContextMock.sol | 7 +++- contracts/utils/Context.sol | 4 ++ contracts/utils/Multicall.sol | 17 +++++++- test/metatx/ERC2771Context.test.js | 55 +++++++++++++++++++++++++- 7 files changed, 110 insertions(+), 12 deletions(-) create mode 100644 .changeset/rude-weeks-beg.md create mode 100644 .changeset/strong-points-invent.md diff --git a/.changeset/rude-weeks-beg.md b/.changeset/rude-weeks-beg.md new file mode 100644 index 000000000..77fe423c6 --- /dev/null +++ b/.changeset/rude-weeks-beg.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC2771Context` and `Context`: Introduce a `_contextPrefixLength()` getter, used to trim extra information appended to `msg.data`. diff --git a/.changeset/strong-points-invent.md b/.changeset/strong-points-invent.md new file mode 100644 index 000000000..980000c42 --- /dev/null +++ b/.changeset/strong-points-invent.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`Multicall`: Make aware of non-canonical context (i.e. `msg.sender` is not `_msgSender()`), allowing compatibility with `ERC2771Context`. diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 38358024c..6acfadffd 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -7,6 +7,10 @@ import "../utils/Context.sol"; /** * @dev Context variant with ERC2771 support. + * + * WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption. + * Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender} + * recovery. */ abstract contract ERC2771Context is Context { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable @@ -21,23 +25,30 @@ abstract contract ERC2771Context is Context { return forwarder == _trustedForwarder; } - function _msgSender() internal view virtual override returns (address sender) { - if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { - // The assembly code is more direct than the Solidity version using `abi.decode`. - /// @solidity memory-safe-assembly - assembly { - sender := shr(96, calldataload(sub(calldatasize(), 20))) - } + function _msgSender() internal view virtual override returns (address) { + uint256 calldataLength = msg.data.length; + uint256 contextSuffixLength = _contextSuffixLength(); + if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) { + return address(bytes20(msg.data[calldataLength - contextSuffixLength:])); } else { return super._msgSender(); } } function _msgData() internal view virtual override returns (bytes calldata) { - if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { - return msg.data[:msg.data.length - 20]; + uint256 calldataLength = msg.data.length; + uint256 contextSuffixLength = _contextSuffixLength(); + if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) { + return msg.data[:calldataLength - contextSuffixLength]; } else { return super._msgData(); } } + + /** + * @dev ERC-2771 specifies the context as being a single address (20 bytes). + */ + function _contextSuffixLength() internal view virtual override returns (uint256) { + return 20; + } } diff --git a/contracts/mocks/ERC2771ContextMock.sol b/contracts/mocks/ERC2771ContextMock.sol index 387df785e..d26a04973 100644 --- a/contracts/mocks/ERC2771ContextMock.sol +++ b/contracts/mocks/ERC2771ContextMock.sol @@ -3,10 +3,11 @@ pragma solidity ^0.8.9; import "./ContextMock.sol"; +import "../utils/Multicall.sol"; import "../metatx/ERC2771Context.sol"; // By inheriting from ERC2771Context, Context's internal functions are overridden automatically -contract ERC2771ContextMock is ContextMock, ERC2771Context { +contract ERC2771ContextMock is ContextMock, ERC2771Context, Multicall { /// @custom:oz-upgrades-unsafe-allow constructor constructor(address trustedForwarder) ERC2771Context(trustedForwarder) { emit Sender(_msgSender()); // _msgSender() should be accessible during construction @@ -19,4 +20,8 @@ contract ERC2771ContextMock is ContextMock, ERC2771Context { function _msgData() internal view override(Context, ERC2771Context) returns (bytes calldata) { return ERC2771Context._msgData(); } + + function _contextSuffixLength() internal view override(Context, ERC2771Context) returns (uint256) { + return ERC2771Context._contextSuffixLength(); + } } diff --git a/contracts/utils/Context.sol b/contracts/utils/Context.sol index f304065b4..5c0b261f2 100644 --- a/contracts/utils/Context.sol +++ b/contracts/utils/Context.sol @@ -21,4 +21,8 @@ abstract contract Context { function _msgData() internal view virtual returns (bytes calldata) { return msg.data; } + + function _contextSuffixLength() internal view virtual returns (uint256) { + return 0; + } } diff --git a/contracts/utils/Multicall.sol b/contracts/utils/Multicall.sol index 5729f8452..88573a8a6 100644 --- a/contracts/utils/Multicall.sol +++ b/contracts/utils/Multicall.sol @@ -4,21 +4,36 @@ pragma solidity ^0.8.0; import "./Address.sol"; +import "./Context.sol"; /** * @dev Provides a function to batch together multiple calls in a single external call. * + * Consider any assumption about calldata validation performed by the sender may be violated if it's not especially + * careful about sending transactions invoking {multicall}. For example, a relay address that filters function + * selectors won't filter calls nested within a {multicall} operation. + * + * NOTE: Since 5.0.1 and 4.9.4, this contract identifies non-canonical contexts (i.e. `msg.sender` is not {_msgSender}). + * If a non-canonical context is identified, the following self `delegatecall` appends the last bytes of `msg.data` + * to the subcall. This makes it safe to use with {ERC2771Context}. Contexts that don't affect the resolution of + * {_msgSender} are not propagated to subcalls. + * * _Available since v4.1._ */ -abstract contract Multicall { +abstract contract Multicall is Context { /** * @dev Receives and executes a batch of function calls on this contract. * @custom:oz-upgrades-unsafe-allow-reachable delegatecall */ function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) { + bytes memory context = msg.sender == _msgSender() + ? new bytes(0) + : msg.data[msg.data.length - _contextSuffixLength():]; + results = new bytes[](data.length); for (uint256 i = 0; i < data.length; i++) { results[i] = Address.functionDelegateCall(address(this), data[i]); + results[i] = Address.functionDelegateCall(address(this), bytes.concat(data[i], context)); } return results; } diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 5108d11cd..bd7387f85 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -12,7 +12,7 @@ const ContextMockCaller = artifacts.require('ContextMockCaller'); const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior'); contract('ERC2771Context', function (accounts) { - const [, trustedForwarder] = accounts; + const [, trustedForwarder, other] = accounts; beforeEach(async function () { this.forwarder = await MinimalForwarder.new(); @@ -118,5 +118,58 @@ contract('ERC2771Context', function (accounts) { const data = recipient.contract.methods.msgDataShort().encodeABI(); await expectEvent(receipt, 'DataShort', { data }); }); + + it('multicall poison attack', async function () { + const attacker = Wallet.generate(); + const attackerAddress = attacker.getChecksumAddressString(); + const nonce = await this.forwarder.getNonce(attackerAddress); + + const msgSenderCall = web3.eth.abi.encodeFunctionCall( + { + name: 'msgSender', + type: 'function', + inputs: [], + }, + [], + ); + + const data = web3.eth.abi.encodeFunctionCall( + { + name: 'multicall', + type: 'function', + inputs: [ + { + internalType: 'bytes[]', + name: 'data', + type: 'bytes[]', + }, + ], + }, + [[web3.utils.encodePacked({ value: msgSenderCall, type: 'bytes' }, { value: other, type: 'address' })]], + ); + + const req = { + from: attackerAddress, + to: this.recipient.address, + value: '0', + gas: '100000', + data, + nonce: Number(nonce), + }; + + const signature = await ethSigUtil.signTypedMessage(attacker.getPrivateKey(), { + data: { + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + message: req, + }, + }); + + expect(await this.forwarder.verify(req, signature)).to.equal(true); + + const receipt = await this.forwarder.execute(req, signature); + await expectEvent.inTransaction(receipt.tx, ERC2771ContextMock, 'Sender', { sender: attackerAddress }); + }); }); });