From fbdb824a735891908d5588b28e0da5852d7ed7ba Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 7 Dec 2023 22:50:28 +0000 Subject: [PATCH] Transpile 01ef4489 --- CHANGELOG.md | 6 ++ .../metatx/ERC2771ContextUpgradeable.sol | 31 ++++++---- .../mocks/ERC2771ContextMockUpgradeable.sol | 7 ++- contracts/package.json | 4 +- contracts/utils/ContextUpgradeable.sol | 6 +- contracts/utils/MulticallUpgradeable.sol | 20 ++++++- lib/openzeppelin-contracts | 2 +- package.json | 2 +- test/metatx/ERC2771Context.test.js | 56 ++++++++++++++++++- 9 files changed, 114 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 249eb76a4..2dee31242 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog + +## 5.0.1 (2023-12-07) + +- `ERC2771Context` and `Context`: Introduce a `_contextPrefixLength()` getter, used to trim extra information appended to `msg.data`. +- `Multicall`: Make aware of non-canonical context (i.e. `msg.sender` is not `_msgSender()`), allowing compatibility with `ERC2771Context`. + ## 5.0.0 (2023-10-05) ### Additions Summary diff --git a/contracts/metatx/ERC2771ContextUpgradeable.sol b/contracts/metatx/ERC2771ContextUpgradeable.sol index fd4eadfc9..7447be513 100644 --- a/contracts/metatx/ERC2771ContextUpgradeable.sol +++ b/contracts/metatx/ERC2771ContextUpgradeable.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (metatx/ERC2771Context.sol) +// OpenZeppelin Contracts (last updated v5.0.1) (metatx/ERC2771Context.sol) pragma solidity ^0.8.20; @@ -14,6 +14,10 @@ import {Initializable} from "../proxy/utils/Initializable.sol"; * 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`. + * + * 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 ERC2771ContextUpgradeable is Initializable, ContextUpgradeable { /// @custom:oz-upgrades-unsafe-allow state-variable-immutable @@ -49,13 +53,11 @@ abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable * a call is not performed by the trusted forwarder or the calldata length is less than * 20 bytes (an address length). */ - 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(); } @@ -67,10 +69,19 @@ abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable * 20 bytes (an address length). */ 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/ERC2771ContextMockUpgradeable.sol b/contracts/mocks/ERC2771ContextMockUpgradeable.sol index 379b41219..2e8c02401 100644 --- a/contracts/mocks/ERC2771ContextMockUpgradeable.sol +++ b/contracts/mocks/ERC2771ContextMockUpgradeable.sol @@ -4,11 +4,12 @@ pragma solidity ^0.8.20; import {ContextMockUpgradeable} from "./ContextMockUpgradeable.sol"; import {ContextUpgradeable} from "../utils/ContextUpgradeable.sol"; +import {MulticallUpgradeable} from "../utils/MulticallUpgradeable.sol"; import {ERC2771ContextUpgradeable} from "../metatx/ERC2771ContextUpgradeable.sol"; import {Initializable} from "../proxy/utils/Initializable.sol"; // By inheriting from ERC2771Context, Context's internal functions are overridden automatically -contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable, ERC2771ContextUpgradeable { +contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable, ERC2771ContextUpgradeable, MulticallUpgradeable { /// @custom:oz-upgrades-unsafe-allow constructor constructor(address trustedForwarder) ERC2771ContextUpgradeable(trustedForwarder) { emit Sender(_msgSender()); // _msgSender() should be accessible during construction @@ -21,4 +22,8 @@ contract ERC2771ContextMockUpgradeable is Initializable, ContextMockUpgradeable, function _msgData() internal view override(ContextUpgradeable, ERC2771ContextUpgradeable) returns (bytes calldata) { return ERC2771ContextUpgradeable._msgData(); } + + function _contextSuffixLength() internal view override(ContextUpgradeable, ERC2771ContextUpgradeable) returns (uint256) { + return ERC2771ContextUpgradeable._contextSuffixLength(); + } } diff --git a/contracts/package.json b/contracts/package.json index 5852484a9..64abf9536 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,7 +1,7 @@ { "name": "@openzeppelin/contracts-upgradeable", "description": "Secure Smart Contract library for Solidity", - "version": "5.0.0", + "version": "5.0.1", "files": [ "**/*.sol", "/build/contracts/*.json", @@ -30,6 +30,6 @@ }, "homepage": "https://openzeppelin.com/contracts/", "peerDependencies": { - "@openzeppelin/contracts": "5.0.0" + "@openzeppelin/contracts": "5.0.1" } } diff --git a/contracts/utils/ContextUpgradeable.sol b/contracts/utils/ContextUpgradeable.sol index f1ab2f3e5..5aa9b48bb 100644 --- a/contracts/utils/ContextUpgradeable.sol +++ b/contracts/utils/ContextUpgradeable.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (utils/Context.sol) +// OpenZeppelin Contracts (last updated v5.0.1) (utils/Context.sol) pragma solidity ^0.8.20; import {Initializable} from "../proxy/utils/Initializable.sol"; @@ -27,4 +27,8 @@ abstract contract ContextUpgradeable is Initializable { 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/MulticallUpgradeable.sol b/contracts/utils/MulticallUpgradeable.sol index 73eabd79b..383258bcb 100644 --- a/contracts/utils/MulticallUpgradeable.sol +++ b/contracts/utils/MulticallUpgradeable.sol @@ -1,15 +1,25 @@ // SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v5.0.0) (utils/Multicall.sol) +// OpenZeppelin Contracts (last updated v5.0.1) (utils/Multicall.sol) pragma solidity ^0.8.20; import {Address} from "@openzeppelin/contracts/utils/Address.sol"; +import {ContextUpgradeable} from "./ContextUpgradeable.sol"; import {Initializable} from "../proxy/utils/Initializable.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. */ -abstract contract MulticallUpgradeable is Initializable { +abstract contract MulticallUpgradeable is Initializable, ContextUpgradeable { function __Multicall_init() internal onlyInitializing { } @@ -20,9 +30,13 @@ abstract contract MulticallUpgradeable is Initializable { * @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/lib/openzeppelin-contracts b/lib/openzeppelin-contracts index 4eb67a408..01ef44898 160000 --- a/lib/openzeppelin-contracts +++ b/lib/openzeppelin-contracts @@ -1 +1 @@ -Subproject commit 4eb67a408c13e8c67f7b99a9d618aa442ad53a84 +Subproject commit 01ef448981be9d20ca85f2faf6ebdf591ce409f3 diff --git a/package.json b/package.json index 8d72d4660..b3d78103e 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "openzeppelin-solidity", "description": "Secure Smart Contract library for Solidity", - "version": "5.0.0", + "version": "5.0.1", "private": true, "files": [ "/contracts/**/*.sol", diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index b0ebccca8..b19e94190 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -13,7 +13,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 ERC2771Forwarder.new('ERC2771Forwarder'); @@ -131,4 +131,58 @@ contract('ERC2771Context', function (accounts) { await expectEvent(receipt, 'DataShort', { data }); }); }); + + it('multicall poison attack', async function () { + const attacker = Wallet.generate(); + const attackerAddress = attacker.getChecksumAddressString(); + const nonce = await this.forwarder.nonces(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), + deadline: MAX_UINT48, + }; + + req.signature = await ethSigUtil.signTypedMessage(attacker.getPrivateKey(), { + data: { + types: this.types, + domain: this.domain, + primaryType: 'ForwardRequest', + message: req, + }, + }); + + expect(await this.forwarder.verify(req)).to.equal(true); + + const receipt = await this.forwarder.execute(req); + await expectEvent.inTransaction(receipt.tx, ERC2771ContextMock, 'Sender', { sender: attackerAddress }); + }); });