Skip to content

Commit

Permalink
prevent calling onCall in execute arb call
Browse files Browse the repository at this point in the history
  • Loading branch information
skosito committed Sep 12, 2024
1 parent 5ac4847 commit b40d43a
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 1 deletion.
10 changes: 10 additions & 0 deletions v2/contracts/evm/GatewayEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ contract GatewayEVM is
/// @param data Calldata to pass to the call.
/// @return The result of the call.
function _executeArbitraryCall(address destination, bytes calldata data) internal returns (bytes memory) {
if (data.length >= 4) {
bytes4 functionSelector;
assembly {
functionSelector := calldataload(data.offset)
}

if (functionSelector == Callable.onCall.selector) {
revert NotAllowedToCallOnCall();
}
}
(bool success, bytes memory result) = destination.call{ value: msg.value }(data);
if (!success) revert ExecutionFailed();

Expand Down
2 changes: 2 additions & 0 deletions v2/contracts/evm/interfaces/IGatewayEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ interface IGatewayEVMErrors {

/// @notice Error when trying to transfer not whitelisted token to custody.
error NotWhitelistedInCustody();

error NotAllowedToCallOnCall();
}

/// @title IGatewayEVM
Expand Down
9 changes: 8 additions & 1 deletion v2/contracts/zevm/GatewayZEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,14 @@ contract GatewayZEVM is
revert GasFeeTransferFailed();
}

emit Called(msg.sender, zrc20, receiver, message, CallOptions({ gasLimit: gasLimit, isArbitraryCall: true }), revertOptions);
emit Called(
msg.sender,
zrc20,
receiver,
message,
CallOptions({ gasLimit: gasLimit, isArbitraryCall: true }),
revertOptions
);
}

/// @notice Deposit foreign coins into ZRC20.
Expand Down
8 changes: 8 additions & 0 deletions v2/test/GatewayEVM.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,14 @@ contract GatewayEVMTest is Test, IGatewayEVMErrors, IGatewayEVMEvents, IReceiver
gateway.execute(address(receiver), data);
}

function testForwardCallToReceiveOnCallFails() public {
bytes memory data = abi.encodeWithSignature("onCall(address,bytes)", address(123), bytes(""));

vm.prank(tssAddress);
vm.expectRevert(NotAllowedToCallOnCall.selector);
gateway.execute(address(receiver), data);
}

function testExecuteFailsIfDestinationIsZeroAddress() public {
bytes memory data = abi.encodeWithSignature("receiveNoParams()");

Expand Down
2 changes: 2 additions & 0 deletions v2/test/utils/IReceiverEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,6 @@ interface IReceiverEVMEvents {
/// @param sender The address of the sender.
/// @param revertContext Revert Context.
event ReceivedRevert(address sender, RevertContext revertContext);

event ReceivedOnCall();
}
4 changes: 4 additions & 0 deletions v2/test/utils/ReceiverEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ contract ReceiverEVM is IReceiverEVMEvents, ReentrancyGuard {
emit ReceivedRevert(msg.sender, revertContext);
}

function onCall(address sender, bytes calldata message) external returns (bytes memory) {
emit ReceivedOnCall();
}

/// @notice Receives ETH.
receive() external payable { }

Expand Down

0 comments on commit b40d43a

Please sign in to comment.