Skip to content

Commit ccfffe1

Browse files
committed
Make Multicall context-aware
1 parent 9329cfa commit ccfffe1

File tree

7 files changed

+110
-12
lines changed

7 files changed

+110
-12
lines changed

.changeset/rude-weeks-beg.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': patch
3+
---
4+
5+
`ERC2771Context` and `Context`: Introduce a `_contextPrefixLength()` getter, used to trim extra information appended to `msg.data`.

.changeset/strong-points-invent.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': patch
3+
---
4+
5+
`Multicall`: Make aware of non-canonical context (i.e. `msg.sender` is not `_msgSender()`), allowing compatibility with `ERC2771Context`.

contracts/metatx/ERC2771Context.sol

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@ import "../utils/Context.sol";
77

88
/**
99
* @dev Context variant with ERC2771 support.
10+
*
11+
* WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
12+
* Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
13+
* recovery.
1014
*/
1115
abstract contract ERC2771Context is Context {
1216
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
@@ -21,23 +25,30 @@ abstract contract ERC2771Context is Context {
2125
return forwarder == _trustedForwarder;
2226
}
2327

24-
function _msgSender() internal view virtual override returns (address sender) {
25-
if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
26-
// The assembly code is more direct than the Solidity version using `abi.decode`.
27-
/// @solidity memory-safe-assembly
28-
assembly {
29-
sender := shr(96, calldataload(sub(calldatasize(), 20)))
30-
}
28+
function _msgSender() internal view virtual override returns (address) {
29+
uint256 calldataLength = msg.data.length;
30+
uint256 contextSuffixLength = _contextSuffixLength();
31+
if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
32+
return address(bytes20(msg.data[calldataLength - contextSuffixLength:]));
3133
} else {
3234
return super._msgSender();
3335
}
3436
}
3537

3638
function _msgData() internal view virtual override returns (bytes calldata) {
37-
if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
38-
return msg.data[:msg.data.length - 20];
39+
uint256 calldataLength = msg.data.length;
40+
uint256 contextSuffixLength = _contextSuffixLength();
41+
if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
42+
return msg.data[:calldataLength - contextSuffixLength];
3943
} else {
4044
return super._msgData();
4145
}
4246
}
47+
48+
/**
49+
* @dev ERC-2771 specifies the context as being a single address (20 bytes).
50+
*/
51+
function _contextSuffixLength() internal view virtual override returns (uint256) {
52+
return 20;
53+
}
4354
}

contracts/mocks/ERC2771ContextMock.sol

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
pragma solidity ^0.8.9;
44

55
import "./ContextMock.sol";
6+
import "../utils/Multicall.sol";
67
import "../metatx/ERC2771Context.sol";
78

89
// By inheriting from ERC2771Context, Context's internal functions are overridden automatically
9-
contract ERC2771ContextMock is ContextMock, ERC2771Context {
10+
contract ERC2771ContextMock is ContextMock, ERC2771Context, Multicall {
1011
/// @custom:oz-upgrades-unsafe-allow constructor
1112
constructor(address trustedForwarder) ERC2771Context(trustedForwarder) {
1213
emit Sender(_msgSender()); // _msgSender() should be accessible during construction
@@ -19,4 +20,8 @@ contract ERC2771ContextMock is ContextMock, ERC2771Context {
1920
function _msgData() internal view override(Context, ERC2771Context) returns (bytes calldata) {
2021
return ERC2771Context._msgData();
2122
}
23+
24+
function _contextSuffixLength() internal view override(Context, ERC2771Context) returns (uint256) {
25+
return ERC2771Context._contextSuffixLength();
26+
}
2227
}

contracts/utils/Context.sol

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,8 @@ abstract contract Context {
2121
function _msgData() internal view virtual returns (bytes calldata) {
2222
return msg.data;
2323
}
24+
25+
function _contextSuffixLength() internal view virtual returns (uint256) {
26+
return 0;
27+
}
2428
}

contracts/utils/Multicall.sol

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,21 +4,36 @@
44
pragma solidity ^0.8.0;
55

66
import "./Address.sol";
7+
import "./Context.sol";
78

89
/**
910
* @dev Provides a function to batch together multiple calls in a single external call.
1011
*
12+
* Consider any assumption about calldata validation performed by the sender may be violated if it's not especially
13+
* careful about sending transactions invoking {multicall}. For example, a relay address that filters function
14+
* selectors won't filter calls nested within a {multicall} operation.
15+
*
16+
* NOTE: Since 5.0.1 and 4.9.4, this contract identifies non-canonical contexts (i.e. `msg.sender` is not {_msgSender}).
17+
* If a non-canonical context is identified, the following self `delegatecall` appends the last bytes of `msg.data`
18+
* to the subcall. This makes it safe to use with {ERC2771Context}. Contexts that don't affect the resolution of
19+
* {_msgSender} are not propagated to subcalls.
20+
*
1121
* _Available since v4.1._
1222
*/
13-
abstract contract Multicall {
23+
abstract contract Multicall is Context {
1424
/**
1525
* @dev Receives and executes a batch of function calls on this contract.
1626
* @custom:oz-upgrades-unsafe-allow-reachable delegatecall
1727
*/
1828
function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) {
29+
bytes memory context = msg.sender == _msgSender()
30+
? new bytes(0)
31+
: msg.data[msg.data.length - _contextSuffixLength():];
32+
1933
results = new bytes[](data.length);
2034
for (uint256 i = 0; i < data.length; i++) {
2135
results[i] = Address.functionDelegateCall(address(this), data[i]);
36+
results[i] = Address.functionDelegateCall(address(this), bytes.concat(data[i], context));
2237
}
2338
return results;
2439
}

test/metatx/ERC2771Context.test.js

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ const ContextMockCaller = artifacts.require('ContextMockCaller');
1212
const { shouldBehaveLikeRegularContext } = require('../utils/Context.behavior');
1313

1414
contract('ERC2771Context', function (accounts) {
15-
const [, trustedForwarder] = accounts;
15+
const [, trustedForwarder, other] = accounts;
1616

1717
beforeEach(async function () {
1818
this.forwarder = await MinimalForwarder.new();
@@ -118,5 +118,58 @@ contract('ERC2771Context', function (accounts) {
118118
const data = recipient.contract.methods.msgDataShort().encodeABI();
119119
await expectEvent(receipt, 'DataShort', { data });
120120
});
121+
122+
it('multicall poison attack', async function () {
123+
const attacker = Wallet.generate();
124+
const attackerAddress = attacker.getChecksumAddressString();
125+
const nonce = await this.forwarder.getNonce(attackerAddress);
126+
127+
const msgSenderCall = web3.eth.abi.encodeFunctionCall(
128+
{
129+
name: 'msgSender',
130+
type: 'function',
131+
inputs: [],
132+
},
133+
[],
134+
);
135+
136+
const data = web3.eth.abi.encodeFunctionCall(
137+
{
138+
name: 'multicall',
139+
type: 'function',
140+
inputs: [
141+
{
142+
internalType: 'bytes[]',
143+
name: 'data',
144+
type: 'bytes[]',
145+
},
146+
],
147+
},
148+
[[web3.utils.encodePacked({ value: msgSenderCall, type: 'bytes' }, { value: other, type: 'address' })]],
149+
);
150+
151+
const req = {
152+
from: attackerAddress,
153+
to: this.recipient.address,
154+
value: '0',
155+
gas: '100000',
156+
data,
157+
nonce: Number(nonce),
158+
};
159+
160+
const signature = await ethSigUtil.signTypedMessage(attacker.getPrivateKey(), {
161+
data: {
162+
types: this.types,
163+
domain: this.domain,
164+
primaryType: 'ForwardRequest',
165+
message: req,
166+
},
167+
});
168+
169+
expect(await this.forwarder.verify(req, signature)).to.equal(true);
170+
171+
const receipt = await this.forwarder.execute(req, signature);
172+
await expectEvent.inTransaction(receipt.tx, ERC2771ContextMock, 'Sender', { sender: attackerAddress });
173+
});
121174
});
122175
});

0 commit comments

Comments
 (0)