Add ERC2771Forwarder fuzz tests for avoiding loss of unused ETH
#4396
Conversation
|
|
Marking as draft until #4346 is merged |
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: ernestognw <ernestognw@gmail.com>
test/metatx/ERC2771Forwarder.t.sol
Outdated
| function testExecuteAvoidsETHStuck(uint256 initialBalance, uint256 value) public { | ||
| vm.assume(initialBalance < _MAX_ETHER); | ||
| vm.assume(value < _MAX_ETHER); | ||
|
|
||
| vm.deal(address(_erc2771Forwarder), initialBalance); | ||
|
|
||
| uint256 nonce = _erc2771Forwarder.nonces(_signer); | ||
|
|
||
| vm.deal(address(this), value); | ||
| _erc2771Forwarder.execute{value: value}( | ||
| _forgeRequestData({ | ||
| value: value, | ||
| nonce: nonce, | ||
| deadline: uint48(block.timestamp + 1), | ||
| data: abi.encodeCall(CallReceiverMock.mockFunction, ()) | ||
| }) | ||
| ); | ||
|
|
||
| assertEq(address(_erc2771Forwarder).balance, initialBalance); | ||
| } |
There was a problem hiding this comment.
This test is fuzzing the initialBalance and the value, but not the "paths" in the execute function. It only runs a function that we know is ok, with a deadline we know is ok, and a nonce we know is correct, with a fixed amount of gas
Ideally, we would want to consider many data, to check that whatever the outcome of the subcall, balances are preserved.
Maybe you could have a uint8 argument that is used to select one of "many" function. mockFunction will only be one of them. It should include: non payable functions, function that revert, function that panic, ...
There was a problem hiding this comment.
or just do 50/50 between mockFunction and mockFunctionRevertsNoReason
There was a problem hiding this comment.
You're right, ideally, we would fuzz multiple functions. However, it becomes difficult to conditionally expect a revert in the test, so I did the 50/50 between mockFunction and mockFunctionRevertNoReason you proposed to cover both paths.
The deadline, nonce and gas are fixed because the contract is about making sure there's no value left in the contract and I don't think there's a way the deadline or nonce might affect that. However, gas may affect it because of the _checkForwardedGas but it's already proven in the comments.
I think the suggestion you proposed for this test is enough for its purpose (not leaving ETH stuck). I also tried to fuzz the gas very early in #4346 but it becomes messier because we have to conditionally expect reverts as well, and it's difficult to predict. Would appreciate if you have any idea how it may work in case you think it's a valuable addition
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Complementary to #4346
PR Checklist
npx changeset add)