Skip to content

Commit fbdb824

Browse files
Transpile 01ef448
1 parent 757a7c8 commit fbdb824

File tree

9 files changed

+114
-20
lines changed

9 files changed

+114
-20
lines changed

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
# Changelog
22

3+
4+
## 5.0.1 (2023-12-07)
5+
6+
- `ERC2771Context` and `Context`: Introduce a `_contextPrefixLength()` getter, used to trim extra information appended to `msg.data`.
7+
- `Multicall`: Make aware of non-canonical context (i.e. `msg.sender` is not `_msgSender()`), allowing compatibility with `ERC2771Context`.
8+
39
## 5.0.0 (2023-10-05)
410

511
### Additions Summary

contracts/metatx/ERC2771ContextUpgradeable.sol

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// SPDX-License-Identifier: MIT
2-
// OpenZeppelin Contracts (last updated v5.0.0) (metatx/ERC2771Context.sol)
2+
// OpenZeppelin Contracts (last updated v5.0.1) (metatx/ERC2771Context.sol)
33

44
pragma solidity ^0.8.20;
55

@@ -14,6 +14,10 @@ import {Initializable} from "../proxy/utils/Initializable.sol";
1414
* specification adding the address size in bytes (20) to the calldata size. An example of an unexpected
1515
* behavior could be an unintended fallback (or another function) invocation while trying to invoke the `receive`
1616
* function only accessible if `msg.data.length == 0`.
17+
*
18+
* WARNING: The usage of `delegatecall` in this contract is dangerous and may result in context corruption.
19+
* Any forwarded request to this contract triggering a `delegatecall` to itself will result in an invalid {_msgSender}
20+
* recovery.
1721
*/
1822
abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable {
1923
/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
@@ -49,13 +53,11 @@ abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable
4953
* a call is not performed by the trusted forwarder or the calldata length is less than
5054
* 20 bytes (an address length).
5155
*/
52-
function _msgSender() internal view virtual override returns (address sender) {
53-
if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
54-
// The assembly code is more direct than the Solidity version using `abi.decode`.
55-
/// @solidity memory-safe-assembly
56-
assembly {
57-
sender := shr(96, calldataload(sub(calldatasize(), 20)))
58-
}
56+
function _msgSender() internal view virtual override returns (address) {
57+
uint256 calldataLength = msg.data.length;
58+
uint256 contextSuffixLength = _contextSuffixLength();
59+
if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
60+
return address(bytes20(msg.data[calldataLength - contextSuffixLength:]));
5961
} else {
6062
return super._msgSender();
6163
}
@@ -67,10 +69,19 @@ abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable
6769
* 20 bytes (an address length).
6870
*/
6971
function _msgData() internal view virtual override returns (bytes calldata) {
70-
if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) {
71-
return msg.data[:msg.data.length - 20];
72+
uint256 calldataLength = msg.data.length;
73+
uint256 contextSuffixLength = _contextSuffixLength();
74+
if (isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength) {
75+
return msg.data[:calldataLength - contextSuffixLength];
7276
} else {
7377
return super._msgData();
7478
}
7579
}
80+
81+
/**
82+
* @dev ERC-2771 specifies the context as being a single address (20 bytes).
83+
*/
84+
function _contextSuffixLength() internal view virtual override returns (uint256) {
85+
return 20;
86+
}
7687
}

contracts/mocks/ERC2771ContextMockUpgradeable.sol

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ pragma solidity ^0.8.20;
44

55
import {ContextMockUpgradeable} from "./ContextMockUpgradeable.sol";
66
import {ContextUpgradeable} from "../utils/ContextUpgradeable.sol";
7+
import {MulticallUpgradeable} from "../utils/MulticallUpgradeable.sol";
78
import {ERC2771ContextUpgradeable} from "../metatx/ERC2771ContextUpgradeable.sol";
89
import {Initializable} from "../proxy/utils/Initializable.sol";
910

1011
// By inheriting from ERC2771Context, Context's internal functions are overridden automatically
11-
contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable, ERC2771ContextUpgradeable {
12+
contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable, ERC2771ContextUpgradeable, MulticallUpgradeable {
1213
/// @custom:oz-upgrades-unsafe-allow constructor
1314
constructor(address trustedForwarder) ERC2771ContextUpgradeable(trustedForwarder) {
1415
emit Sender(_msgSender()); // _msgSender() should be accessible during construction
@@ -21,4 +22,8 @@ contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable,
2122
function _msgData() internal view override(ContextUpgradeable, ERC2771ContextUpgradeable) returns (bytes calldata) {
2223
return ERC2771ContextUpgradeable._msgData();
2324
}
25+
26+
function _contextSuffixLength() internal view override(ContextUpgradeable, ERC2771ContextUpgradeable) returns (uint256) {
27+
return ERC2771ContextUpgradeable._contextSuffixLength();
28+
}
2429
}

contracts/package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "@openzeppelin/contracts-upgradeable",
33
"description": "Secure Smart Contract library for Solidity",
4-
"version": "5.0.0",
4+
"version": "5.0.1",
55
"files": [
66
"**/*.sol",
77
"/build/contracts/*.json",
@@ -30,6 +30,6 @@
3030
},
3131
"homepage": "https://openzeppelin.com/contracts/",
3232
"peerDependencies": {
33-
"@openzeppelin/contracts": "5.0.0"
33+
"@openzeppelin/contracts": "5.0.1"
3434
}
3535
}

contracts/utils/ContextUpgradeable.sol

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// SPDX-License-Identifier: MIT
2-
// OpenZeppelin Contracts (last updated v5.0.0) (utils/Context.sol)
2+
// OpenZeppelin Contracts (last updated v5.0.1) (utils/Context.sol)
33

44
pragma solidity ^0.8.20;
55
import {Initializable} from "../proxy/utils/Initializable.sol";
@@ -27,4 +27,8 @@ abstract contract ContextUpgradeable is Initializable {
2727
function _msgData() internal view virtual returns (bytes calldata) {
2828
return msg.data;
2929
}
30+
31+
function _contextSuffixLength() internal view virtual returns (uint256) {
32+
return 0;
33+
}
3034
}

contracts/utils/MulticallUpgradeable.sol

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,25 @@
11
// SPDX-License-Identifier: MIT
2-
// OpenZeppelin Contracts (last updated v5.0.0) (utils/Multicall.sol)
2+
// OpenZeppelin Contracts (last updated v5.0.1) (utils/Multicall.sol)
33

44
pragma solidity ^0.8.20;
55

66
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
7+
import {ContextUpgradeable} from "./ContextUpgradeable.sol";
78
import {Initializable} from "../proxy/utils/Initializable.sol";
89

910
/**
1011
* @dev Provides a function to batch together multiple calls in a single external call.
12+
*
13+
* Consider any assumption about calldata validation performed by the sender may be violated if it's not especially
14+
* careful about sending transactions invoking {multicall}. For example, a relay address that filters function
15+
* selectors won't filter calls nested within a {multicall} operation.
16+
*
17+
* NOTE: Since 5.0.1 and 4.9.4, this contract identifies non-canonical contexts (i.e. `msg.sender` is not {_msgSender}).
18+
* If a non-canonical context is identified, the following self `delegatecall` appends the last bytes of `msg.data`
19+
* to the subcall. This makes it safe to use with {ERC2771Context}. Contexts that don't affect the resolution of
20+
* {_msgSender} are not propagated to subcalls.
1121
*/
12-
abstract contract MulticallUpgradeable is Initializable {
22+
abstract contract MulticallUpgradeable is Initializable, ContextUpgradeable {
1323
function __Multicall_init() internal onlyInitializing {
1424
}
1525

@@ -20,9 +30,13 @@ abstract contract MulticallUpgradeable is Initializable {
2030
* @custom:oz-upgrades-unsafe-allow-reachable delegatecall
2131
*/
2232
function multicall(bytes[] calldata data) external virtual returns (bytes[] memory results) {
33+
bytes memory context = msg.sender == _msgSender()
34+
? new bytes(0)
35+
: msg.data[msg.data.length - _contextSuffixLength():];
36+
2337
results = new bytes[](data.length);
2438
for (uint256 i = 0; i < data.length; i++) {
25-
results[i] = Address.functionDelegateCall(address(this), data[i]);
39+
results[i] = Address.functionDelegateCall(address(this), bytes.concat(data[i], context));
2640
}
2741
return results;
2842
}

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "openzeppelin-solidity",
33
"description": "Secure Smart Contract library for Solidity",
4-
"version": "5.0.0",
4+
"version": "5.0.1",
55
"private": true,
66
"files": [
77
"/contracts/**/*.sol",

test/metatx/ERC2771Context.test.js

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

1515
contract('ERC2771Context', function (accounts) {
16-
const [, trustedForwarder] = accounts;
16+
const [, trustedForwarder, other] = accounts;
1717

1818
beforeEach(async function () {
1919
this.forwarder = await ERC2771Forwarder.new('ERC2771Forwarder');
@@ -131,4 +131,58 @@ contract('ERC2771Context', function (accounts) {
131131
await expectEvent(receipt, 'DataShort', { data });
132132
});
133133
});
134+
135+
it('multicall poison attack', async function () {
136+
const attacker = Wallet.generate();
137+
const attackerAddress = attacker.getChecksumAddressString();
138+
const nonce = await this.forwarder.nonces(attackerAddress);
139+
140+
const msgSenderCall = web3.eth.abi.encodeFunctionCall(
141+
{
142+
name: 'msgSender',
143+
type: 'function',
144+
inputs: [],
145+
},
146+
[],
147+
);
148+
149+
const data = web3.eth.abi.encodeFunctionCall(
150+
{
151+
name: 'multicall',
152+
type: 'function',
153+
inputs: [
154+
{
155+
internalType: 'bytes[]',
156+
name: 'data',
157+
type: 'bytes[]',
158+
},
159+
],
160+
},
161+
[[web3.utils.encodePacked({ value: msgSenderCall, type: 'bytes' }, { value: other, type: 'address' })]],
162+
);
163+
164+
const req = {
165+
from: attackerAddress,
166+
to: this.recipient.address,
167+
value: '0',
168+
gas: '100000',
169+
data,
170+
nonce: Number(nonce),
171+
deadline: MAX_UINT48,
172+
};
173+
174+
req.signature = await ethSigUtil.signTypedMessage(attacker.getPrivateKey(), {
175+
data: {
176+
types: this.types,
177+
domain: this.domain,
178+
primaryType: 'ForwardRequest',
179+
message: req,
180+
},
181+
});
182+
183+
expect(await this.forwarder.verify(req)).to.equal(true);
184+
185+
const receipt = await this.forwarder.execute(req);
186+
await expectEvent.inTransaction(receipt.tx, ERC2771ContextMock, 'Sender', { sender: attackerAddress });
187+
});
134188
});

0 commit comments

Comments
 (0)