Hello, after reading the disclosure article, there is still a hole in my understanding of the implementation changes regarding the ERC2771Context and Multicall usage.
How I understand the problem
An attacker can create a call to multicall() with malicious calldata such as a transfer call to the attacker for x amount of some token.
Next, the attacker adds the victim's address as the last 20 bytes to the call data (per ERC2771 spec)
Later on, the trusted forwarder calls the ERC2771ContextMulticall contract and since msg.sender is the Trusted forwarder, the OZ's _msgSender() is set to the last 20 bytes of the calldata (which at this point is forged by the attacker)
The vulnerable ERC2771ContextMulticall delegate calls itself with the forged caller address. Funds are stolen.
What I'm not understanding
Non-canonical context happens when msg.sender is not _msgSender() and in case of ERC2771ContextMulticall contract the msg.sender will be the trusted forwarder and _msgSender() will be the extracted 20 bytes of the calldata, which could be any address (for brevity, assuming here that isTrustedForwarder(msg.sender) && calldataLength >= contextSuffixLength check passes )
Since the two addresses above are not expected to be the same bytes memory context will be set to msg.data[msg.data.length - _contextSuffixLength():]; aka the last 20 bytes (which, just like above, were forged by the attacker).
Now, when the delegate call in ERC2771ContextMulticall contract happens those 20 bytes are appended to the call data (i.e. results[i] = Address.functionDelegateCall(address(this), bytes.concat(data[i], context));)
And this is what confuses me, in both cases (pre v4.9.4/5.0.1 fix and before) we have the forged caller address added to the end of the calldata (per ERC2771 spec). How exactly does this prevent the attack since in both cases the last 20 bytes are taken directly from the forged calldata?
Can someone help me understand this little better?