Skip to content

Commit 5ae86d3

Browse files
izquiizqui
authored
Relayer: increase nonce even if execution fails (#180)
* Relayer: increase nonce even if execution fails * More tests * Relayer: prevent return gas bomb (#181)
1 parent 48b463c commit 5ae86d3

2 files changed

Lines changed: 135 additions & 26 deletions

File tree

‎src/metatx/FirmRelayer.sol‎

Lines changed: 70 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ contract FirmRelayer is EIP712 {
5656
bytes32 internal constant ZERO_HASH = 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470;
5757

5858
uint256 internal constant ASSERTION_WORD_SIZE = 32;
59+
uint256 internal constant RELAY_GAS_BUFFER = 10000;
60+
uint256 internal constant MAX_REVERT_DATA = 320;
5961

6062
mapping(address => uint256) public getNonce;
6163

@@ -64,11 +66,14 @@ contract FirmRelayer is EIP712 {
6466
error CallExecutionFailed(uint256 callIndex, address to, bytes revertData);
6567
error BadAssertionIndex(uint256 callIndex);
6668
error AssertionPositionOutOfBounds(uint256 callIndex, uint256 returnDataLenght);
67-
error AssertionFailed(uint256 callIndex, bytes32 actualValue, bytes32 expectedValue);
69+
error UnexpectedReturnValue(uint256 callIndex, bytes32 actualValue, bytes32 expectedValue);
6870
error UnauthorizedSenderNotFrom();
71+
error InsufficientGas();
72+
error BadExecutionContext();
6973

7074
event Relayed(address indexed relayer, address indexed signer, uint256 nonce, uint256 numCalls);
7175
event SelfRelayed(address indexed sender, uint256 numCalls);
76+
event RelayExecutionFailed(address indexed relayer, address indexed signer, uint256 nonce, bytes revertData);
7277

7378
constructor() EIP712("Firm Relayer", "0.0.1") {}
7479

@@ -100,9 +105,35 @@ contract FirmRelayer is EIP712 {
100105
}
101106
getNonce[signer] = request.nonce + 1;
102107

103-
_execute(signer, request.calls, request.assertions);
108+
// We check how much gas all calls are going to use and make sure we have enough
109+
// This is to ensure that the external execute call will not fail due to OOG
110+
// which would allow to block the request by forcing it to fail
111+
uint256 callsGas = 0;
112+
uint256 callsLength = request.calls.length;
113+
for (uint256 i = 0; i < callsLength;) {
114+
callsGas += request.calls[i].gas;
115+
unchecked {
116+
i++;
117+
}
118+
}
119+
120+
if (gasleft() < callsGas + RELAY_GAS_BUFFER) {
121+
revert InsufficientGas();
122+
}
123+
124+
// We perform the execution as an external call so if the execution fails,
125+
// everything that happened in that sub-call is reverted, but not this
126+
// top-level call. This is important because we don't want to revert the
127+
// nonce increase if the execution fails.
128+
(bool ok, bytes memory returnData) = address(this).call(
129+
abi.encodeWithSelector(this.__externalSelfCall_execute.selector, signer, request.calls, request.assertions)
130+
);
104131

105-
emit Relayed(msg.sender, signer, request.nonce, request.calls.length);
132+
if (ok) {
133+
emit Relayed(msg.sender, signer, request.nonce, request.calls.length);
134+
} else {
135+
emit RelayExecutionFailed(msg.sender, signer, request.nonce, returnData);
136+
}
106137
}
107138

108139
/**
@@ -120,14 +151,40 @@ contract FirmRelayer is EIP712 {
120151
emit SelfRelayed(msg.sender, calls.length);
121152
}
122153

154+
function __externalSelfCall_execute(address asSender, Call[] calldata calls, Assertion[] calldata assertions) external {
155+
if (msg.sender != address(this)) {
156+
revert BadExecutionContext();
157+
}
158+
159+
_execute(asSender, calls, assertions);
160+
}
161+
123162
function _execute(address asSender, Call[] calldata calls, Assertion[] calldata assertions) internal {
124163
for (uint256 i = 0; i < calls.length;) {
125164
Call calldata call = calls[i];
126165

166+
address to = call.to;
167+
uint256 value = call.value;
168+
uint256 callGas = call.gas;
127169
bytes memory payload = abi.encodePacked(call.data, asSender);
128-
(bool success, bytes memory returnData) = call.to.call{value: call.value, gas: call.gas}(payload);
170+
uint256 returnDataSize;
171+
bool success;
172+
173+
/// @solidity memory-safe-assembly
174+
assembly {
175+
success := call(callGas, to, value, add(payload, 0x20), mload(payload), 0, 0)
176+
returnDataSize := returndatasize()
177+
}
178+
129179
if (!success) {
130-
revert CallExecutionFailed(i, call.to, returnData);
180+
// Prevent revert data from being too large
181+
uint256 revertDataSize = returnDataSize > MAX_REVERT_DATA ? MAX_REVERT_DATA : returnDataSize;
182+
bytes memory revertData = new bytes(revertDataSize);
183+
/// @solidity memory-safe-assembly
184+
assembly {
185+
returndatacopy(add(revertData, 0x20), 0, revertDataSize)
186+
}
187+
revert CallExecutionFailed(i, call.to, revertData);
131188
}
132189

133190
uint256 assertionIndex = call.assertionIndex;
@@ -137,20 +194,21 @@ contract FirmRelayer is EIP712 {
137194
}
138195

139196
Assertion calldata assertion = assertions[assertionIndex - 1];
140-
uint256 returnDataMinLength = assertion.position + ASSERTION_WORD_SIZE;
141-
if (returnDataMinLength > returnData.length) {
142-
revert AssertionPositionOutOfBounds(i, returnData.length);
197+
uint256 assertionPosition = assertion.position;
198+
if (assertion.position + ASSERTION_WORD_SIZE > returnDataSize) {
199+
revert AssertionPositionOutOfBounds(i, returnDataSize);
143200
}
144201

202+
// Only copy the return data word we need to check
145203
bytes32 returnValue;
146204
/// @solidity memory-safe-assembly
147205
assembly {
148-
// Position in memory for the value to be read is returnData + 0x20 + position
149-
// so we can reuse returnDataMinLength (position + 32) from above as an optimization
150-
returnValue := mload(add(returnData, returnDataMinLength))
206+
let copyPosition := mload(0x40)
207+
returndatacopy(copyPosition, assertionPosition, ASSERTION_WORD_SIZE)
208+
returnValue := mload(copyPosition)
151209
}
152210
if (returnValue != assertion.expectedValue) {
153-
revert AssertionFailed(i, returnValue, assertion.expectedValue);
211+
revert UnexpectedReturnValue(i, returnValue, assertion.expectedValue);
154212
}
155213
}
156214
unchecked {

‎src/metatx/test/FirmRelayer.t.sol‎

Lines changed: 65 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -104,22 +104,33 @@ contract FirmRelayerTest is FirmTest {
104104
relayer.relay(request, _signPacked(hash, USER_PK));
105105
}
106106

107-
function testRevertOnTargetBadSender() public {
107+
function testExecutionCallRevertsAllCalls() public {
108108
(address otherUser, uint256 otherUserPk) = accountAndKey("other user");
109109

110-
FirmRelayer.Call memory call = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (USER)));
111-
FirmRelayer.RelayRequest memory request = _defaultRequestWithCall(call);
110+
FirmRelayer.Call memory call1 = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (otherUser)));
111+
FirmRelayer.Call memory call2 = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (USER)));
112+
113+
FirmRelayer.RelayRequest memory request = _defaultRequestWithCall(call1);
114+
request.calls = new FirmRelayer.Call[](2);
115+
request.calls[0] = call1;
116+
request.calls[1] = call2;
112117
request.from = otherUser;
113118
bytes32 hash = relayer.requestTypedDataHash(request);
114119

115120
bytes memory targetError = abi.encodeWithSelector(RelayTarget.BadSender.selector, USER, otherUser);
116-
vm.expectRevert(
117-
abi.encodeWithSelector(FirmRelayer.CallExecutionFailed.selector, 0, address(target), targetError)
121+
assertFailureEvent(
122+
otherUser,
123+
abi.encodeWithSelector(FirmRelayer.CallExecutionFailed.selector, 1, address(target), targetError)
118124
);
119125
relayer.relay(request, _signPacked(hash, otherUserPk));
126+
127+
// Nonce should have been incremented
128+
assertEq(relayer.getNonce(otherUser), 1);
129+
// Successful call on target should have been reverted
130+
assertEq(target.lastSender(), address(0));
120131
}
121132

122-
function testRevertOnAssertionFailure() public {
133+
function testExecutionRevertOnAssertionFailure() public {
123134
bytes32 actualReturnValue = bytes32(abi.encode(USER));
124135
bytes32 badExpectedValue = bytes32(uint256(0));
125136

@@ -128,31 +139,60 @@ contract FirmRelayerTest is FirmTest {
128139
FirmRelayer.RelayRequest memory request = _defaultRequestWithCallAndAssertion(call, assertion);
129140

130141
bytes32 hash = relayer.requestTypedDataHash(request);
131-
vm.expectRevert(
132-
abi.encodeWithSelector(FirmRelayer.AssertionFailed.selector, 0, actualReturnValue, badExpectedValue)
133-
);
142+
143+
assertFailureEvent(USER, abi.encodeWithSelector(FirmRelayer.UnexpectedReturnValue.selector, 0, actualReturnValue, badExpectedValue));
134144
relayer.relay(request, _signPacked(hash, USER_PK));
145+
146+
// Nonce should have been incremented
147+
assertEq(relayer.getNonce(USER), 1);
135148
}
136149

137-
function testRevertOnAssertionOutOfBounds() public {
150+
function testExecutionRevertOnAssertionOutOfBounds() public {
138151
FirmRelayer.Call memory call = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (USER)));
139152
FirmRelayer.Assertion memory assertion = FirmRelayer.Assertion(1, bytes32(abi.encode(USER)));
140153
FirmRelayer.RelayRequest memory request = _defaultRequestWithCallAndAssertion(call, assertion);
141154

142155
bytes32 hash = relayer.requestTypedDataHash(request);
143-
vm.expectRevert(abi.encodeWithSelector(FirmRelayer.AssertionPositionOutOfBounds.selector, 0, 32));
156+
assertFailureEvent(USER, abi.encodeWithSelector(FirmRelayer.AssertionPositionOutOfBounds.selector, 0, 32));
144157
relayer.relay(request, _signPacked(hash, USER_PK));
158+
159+
// Nonce should have been incremented
160+
assertEq(relayer.getNonce(USER), 1);
145161
}
146162

147-
function testRevertOnBadAssertionIndex() public {
163+
function testExecutionRevertOnBadAssertionIndex() public {
148164
FirmRelayer.Call memory call = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (USER)));
149165
FirmRelayer.Assertion memory assertion = FirmRelayer.Assertion(0, bytes32(abi.encode(USER)));
150166
FirmRelayer.RelayRequest memory request = _defaultRequestWithCallAndAssertion(call, assertion);
151167
request.calls[0].assertionIndex = 2;
152168

153169
bytes32 hash = relayer.requestTypedDataHash(request);
154-
vm.expectRevert(abi.encodeWithSelector(FirmRelayer.BadAssertionIndex.selector, 0));
170+
assertFailureEvent(USER, abi.encodeWithSelector(FirmRelayer.BadAssertionIndex.selector, 0));
155171
relayer.relay(request, _signPacked(hash, USER_PK));
172+
173+
// Nonce should have been incremented
174+
assertEq(relayer.getNonce(USER), 1);
175+
}
176+
177+
function testRevertOnInsufficientGas() public {
178+
FirmRelayer.Call memory call = _defaultCallWithData(address(target), abi.encodeCall(target.onlySender, (USER)));
179+
FirmRelayer.RelayRequest memory request = _defaultRequestWithCall(call);
180+
bytes memory sig = _signPacked(relayer.requestTypedDataHash(request), USER_PK);
181+
182+
vm.expectRevert(abi.encodeWithSelector(FirmRelayer.InsufficientGas.selector));
183+
relayer.relay{ gas: call.gas - 100 }(request, sig);
184+
185+
// Nonce not incremented, can relay the same request again
186+
assertEq(relayer.getNonce(USER), 0);
187+
188+
// Relay should succeed with enough gas (account for buffer)
189+
relayer.relay{ gas: call.gas + 40000 }(request, sig);
190+
assertEq(target.lastSender(), USER);
191+
}
192+
193+
function testRevertOnExternalSelfExecute() public {
194+
vm.expectRevert(abi.encodeWithSelector(FirmRelayer.BadExecutionContext.selector));
195+
relayer.__externalSelfCall_execute(address(this), new FirmRelayer.Call[](0), new FirmRelayer.Assertion[](0));
156196
}
157197

158198
function testSelfRelay() public {
@@ -181,7 +221,7 @@ contract FirmRelayerTest is FirmTest {
181221
call.to = to;
182222
call.data = data;
183223
call.value = 0;
184-
call.gas = 10_000_000; // random big value for testing
224+
call.gas = 200_000; // random big value for testing
185225
call.assertionIndex = 0;
186226
}
187227

@@ -207,6 +247,17 @@ contract FirmRelayerTest is FirmTest {
207247
request.calls[0].assertionIndex = 1;
208248
}
209249

250+
event RelayExecutionFailed(address indexed relayer, address indexed signer, uint256 nonce, bytes revertData);
251+
function assertFailureEvent(address sender, bytes memory revertData) internal {
252+
vm.expectEmit(true, true, true, true, address(relayer));
253+
emit RelayExecutionFailed(
254+
address(this),
255+
sender,
256+
0,
257+
revertData
258+
);
259+
}
260+
210261
// For this test we need to fix both the address of FirmRelayer and chainId so that the hash
211262
// matches a hash generated off-chain with those parameters
212263
// Gen script: https://gist.github.com/izqui/f0379eb81c5e46f79696f88736ce1ffa

0 commit comments

Comments
 (0)