Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prevent on revert spoofing backport #362

Merged
merged 6 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/generated-files_v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
- "v2/**"
pull_request:
branches:
- "*"
- "**"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workflows were not triggering, maybe because of branch naming, but with ** seems to work, will include this on main branch as well

paths:
- "v2/**"
types:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lint_v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
- 'v2/**'
pull_request:
branches:
- "*"
- "**"
paths:
- 'v2/**'
types:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/slither_v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
- 'v2/**'
pull_request:
branches:
- "*"
- "**"
paths:
- 'v2/**'
types:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test_v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
- 'v2/**'
pull_request:
branches:
- "*"
- "**"
paths:
- 'v2/**'
types:
Expand Down
15 changes: 15 additions & 0 deletions v2/contracts/evm/GatewayEVM.sol
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ contract GatewayEVM is
/// @param data Calldata to pass to the call.
/// @return The result of the call.
function _execute(address destination, bytes calldata data) internal returns (bytes memory) {
revertIfCallingOnRevert(data);
(bool success, bytes memory result) = destination.call{ value: msg.value }(data);
if (!success) revert ExecutionFailed();

Expand Down Expand Up @@ -385,4 +386,18 @@ contract GatewayEVM is
IERC20(token).safeTransfer(custody, amount);
}
}

// @dev prevent spoofing onRevert functions
function revertIfCallingOnRevert(bytes calldata data) private pure {
if (data.length >= 4) {
bytes4 functionSelector;
assembly {
functionSelector := calldataload(data.offset)
}

if (functionSelector == Revertable.onRevert.selector) {
revert NotAllowedToCallOnRevert();
}
Comment on lines +398 to +400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: is it possible to update this to Solidity 0.8.27? In that version require can now exit with a custom error, rendering this expression to:

Suggested change
if (functionSelector == Revertable.onRevert.selector) {
revert NotAllowedToCallOnRevert();
}
require(functionSelector != Revertable.onRevert.selector, NotAllowedToCallOnRevert())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably can, but in separate effort, this PR is for backporting a fix, please open issue for that update

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

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

/// @notice Error when trying to call onRevert method using arbitrary call.
error NotAllowedToCallOnRevert();
}

/// @title IGatewayEVM
Expand Down
2 changes: 1 addition & 1 deletion v2/pkg/erc20custody.sol/erc20custody.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/erc20custody.t.sol/erc20custodytest.go

Large diffs are not rendered by default.

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevm.sol/gatewayevm.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevm.t.sol/gatewayevminboundtest.go

Large diffs are not rendered by default.

25 changes: 23 additions & 2 deletions v2/pkg/gatewayevm.t.sol/gatewayevmtest.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevmechidnatest.sol/gatewayevmechidnatest.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevmupgrade.t.sol/gatewayevmuupsupgradetest.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevmupgradetest.sol/gatewayevmupgradetest.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/gatewayevmzevm.t.sol/gatewayevmzevmtest.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion v2/pkg/igatewayevm.sol/igatewayevm.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion v2/pkg/igatewayevm.sol/igatewayevmerrors.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion v2/pkg/zetaconnectornative.sol/zetaconnectornative.go

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions v2/pkg/zetaconnectornative.t.sol/zetaconnectornativetest.go

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Large diffs are not rendered by default.

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 testForwardCallToReceiveOnRevertFails() public {
bytes memory data = abi.encodeWithSignature("onRevert((address,uint64,bytes))");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this data build when we don't provide value for address,uint64,bytes? Is it using the 0 value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think its using default values for the type


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

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

Expand Down
2 changes: 1 addition & 1 deletion v2/types/factories/ERC20CustodyEchidnaTest__factory.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion v2/types/factories/ERC20Custody__factory.ts

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion v2/types/factories/GatewayEVMEchidnaTest__factory.ts

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion v2/types/factories/GatewayEVMUpgradeTest__factory.ts

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion v2/types/factories/GatewayEVM__factory.ts

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ const _abi = [
name: "InsufficientETHAmount",
inputs: [],
},
{
type: "error",
name: "NotAllowedToCallOnRevert",
inputs: [],
},
{
type: "error",
name: "NotWhitelistedInCustody",
Expand Down
5 changes: 5 additions & 0 deletions v2/types/factories/IGatewayEVM.sol/IGatewayEVM__factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,11 @@ const _abi = [
name: "InsufficientETHAmount",
inputs: [],
},
{
type: "error",
name: "NotAllowedToCallOnRevert",
inputs: [],
},
{
type: "error",
name: "NotWhitelistedInCustody",
Expand Down
2 changes: 1 addition & 1 deletion v2/types/factories/ZetaConnectorNative__factory.ts

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion v2/types/factories/ZetaConnectorNonNative__factory.ts

Large diffs are not rendered by default.

Loading