From 642273cfb92c27bdc5400c96ed91286ec6a97186 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 26 Jul 2023 15:35:13 -0300 Subject: [PATCH 1/3] Adjust ERC2771Context._msgData for msg.data.length < 20 --- contracts/metatx/ERC2771Context.sol | 2 +- contracts/mocks/ContextMock.sol | 6 ++++++ test/metatx/ERC2771Context.test.js | 18 ++++++++++++++---- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/contracts/metatx/ERC2771Context.sol b/contracts/metatx/ERC2771Context.sol index 9ade6148d39..2c4d89f94cb 100644 --- a/contracts/metatx/ERC2771Context.sol +++ b/contracts/metatx/ERC2771Context.sol @@ -34,7 +34,7 @@ abstract contract ERC2771Context is Context { } function _msgData() internal view virtual override returns (bytes calldata) { - if (isTrustedForwarder(msg.sender)) { + if (isTrustedForwarder(msg.sender) && msg.data.length >= 20) { return msg.data[:msg.data.length - 20]; } else { return super._msgData(); diff --git a/contracts/mocks/ContextMock.sol b/contracts/mocks/ContextMock.sol index fd105aa9142..fb57535f706 100644 --- a/contracts/mocks/ContextMock.sol +++ b/contracts/mocks/ContextMock.sol @@ -16,6 +16,12 @@ contract ContextMock is Context { function msgData(uint256 integerValue, string memory stringValue) public { emit Data(_msgData(), integerValue, stringValue); } + + event DataShort(bytes data); + + function msgDataShort() public { + emit DataShort(_msgData()); + } } contract ContextMockCaller { diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index a907e502cdd..9f5c3ab7ae9 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 [, anotherAccount] = accounts; + const [, trustedForwarder] = accounts; const MAX_UINT48 = web3.utils.toBN(1).shln(48).subn(1).toString(); @@ -84,11 +84,11 @@ contract('ERC2771Context', function (accounts) { it('returns the original sender when calldata length is less than 20 bytes (address length)', async function () { // The forwarder doesn't produce calls with calldata length less than 20 bytes - const recipient = await ERC2771ContextMock.new(anotherAccount); + const recipient = await ERC2771ContextMock.new(trustedForwarder); - const { receipt } = await recipient.msgSender({ from: anotherAccount }); + const { receipt } = await recipient.msgSender({ from: trustedForwarder }); - await expectEvent(receipt, 'Sender', { sender: anotherAccount }); + await expectEvent(receipt, 'Sender', { sender: trustedForwarder }); }); }); @@ -117,5 +117,15 @@ contract('ERC2771Context', function (accounts) { await expectEvent.inTransaction(tx, ERC2771ContextMock, 'Data', { data, integerValue, stringValue }); }); }); + + it.only('returns the full original data when calldata length is less than 20 bytes (address length)', async function () { + // The forwarder doesn't produce calls with calldata length less than 20 bytes + const recipient = await ERC2771ContextMock.new(trustedForwarder); + + const { receipt } = await recipient.msgDataShort({ from: trustedForwarder }); + + const data = recipient.contract.methods.msgDataShort().encodeABI(); + await expectEvent(receipt, 'DataShort', { data }); + }); }); }); From 0f697db85d6c578eeade49808f93af4362a3754c Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 26 Jul 2023 15:43:22 -0300 Subject: [PATCH 2/3] add changeset --- .changeset/warm-guests-rule.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/warm-guests-rule.md diff --git a/.changeset/warm-guests-rule.md b/.changeset/warm-guests-rule.md new file mode 100644 index 00000000000..049da4dd5d5 --- /dev/null +++ b/.changeset/warm-guests-rule.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`ERC2771Context`: Prevent revert in `_msgData()` when a call originating from a trusted forwarder is not long enough to contain the request signer address (i.e. `msg.data.length` is less than 20 bytes). Return the full calldata in that case. From 507e7bafbd37d415b8efe4307167e0e4c8239455 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 26 Jul 2023 15:43:39 -0300 Subject: [PATCH 3/3] remove .only --- test/metatx/ERC2771Context.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/metatx/ERC2771Context.test.js b/test/metatx/ERC2771Context.test.js index 9f5c3ab7ae9..0ec8d98dde6 100644 --- a/test/metatx/ERC2771Context.test.js +++ b/test/metatx/ERC2771Context.test.js @@ -118,7 +118,7 @@ contract('ERC2771Context', function (accounts) { }); }); - it.only('returns the full original data when calldata length is less than 20 bytes (address length)', async function () { + it('returns the full original data when calldata length is less than 20 bytes (address length)', async function () { // The forwarder doesn't produce calls with calldata length less than 20 bytes const recipient = await ERC2771ContextMock.new(trustedForwarder);