Skip to content

onlyGateway modifier #213

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 1, 2024
+54 −16
Merged

onlyGateway modifier #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f3ef52
onlyGateway modifier
fadeev Nov 1, 2024
3c71726
check balance after checking localnet
fadeev Nov 1, 2024
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
9 changes: 8 additions & 1 deletion examples/hello/contracts/Echo.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ contract Echo {
event RevertEvent(string, RevertContext);
event HelloEvent(string, string);

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(address payable gatewayAddress) {
gateway = GatewayEVM(gatewayAddress);
}
Expand All @@ -18,7 +23,9 @@ contract Echo {
emit HelloEvent("Hello on EVM", message);
}

function onRevert(RevertContext calldata revertContext) external {
function onRevert(
RevertContext calldata revertContext
) external onlyGateway {
emit RevertEvent("Revert on EVM", revertContext);
}

Expand Down
11 changes: 9 additions & 2 deletions examples/hello/contracts/Hello.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ contract Hello is UniversalContract {
event RevertEvent(string, RevertContext);
error TransferFailed();

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(address payable gatewayAddress) {
gateway = GatewayZEVM(gatewayAddress);
}
Expand All @@ -22,12 +27,14 @@ contract Hello is UniversalContract {
address zrc20,
uint256 amount,
bytes calldata message
) external override {
) external override onlyGateway {
string memory name = abi.decode(message, (string));
emit HelloEvent("Hello on ZetaChain", name);
}

function onRevert(RevertContext calldata revertContext) external override {
function onRevert(
RevertContext calldata revertContext
) external override onlyGateway {
emit RevertEvent("Revert on ZetaChain", revertContext);
}

Expand Down
9 changes: 7 additions & 2 deletions examples/nft/contracts/Connected.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {
counterparty = contractAddress;
}

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(
address payable gatewayAddress,
address initialOwner
Expand Down Expand Up @@ -74,7 +79,7 @@ contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {
function onCall(
MessageContext calldata messageContext,
bytes calldata message
) external payable returns (bytes4) {
) external payable onlyGateway returns (bytes4) {
if (messageContext.sender != counterparty) revert("Unauthorized");

(uint256 tokenId, address receiver, string memory uri) = abi.decode(
Expand All @@ -86,7 +91,7 @@ contract Connected is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable {
return "";
}

function onRevert(RevertContext calldata context) external {
function onRevert(RevertContext calldata context) external onlyGateway {
(uint256 tokenId, address sender, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
Expand Down
9 changes: 7 additions & 2 deletions examples/nft/contracts/Universal.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ contract Universal is

event CounterpartySet(address indexed zrc20, bytes indexed contractAddress);

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(
address payable gatewayAddress,
address initialOwner
Expand Down Expand Up @@ -102,7 +107,7 @@ contract Universal is
address zrc20,
uint256 amount,
bytes calldata message
) external override {
) external override onlyGateway {
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

🛠️ Refactor suggestion

Hardcoded gas limit needs to be replaced with state variable

The review comment is partially correct. After analyzing the contract:

  1. The contract has a state variable gasLimit initialized to 700000, but the onCall function uses the hardcoded value instead of this configurable variable.
  2. However, regarding reentrancy concerns:
    • The function follows CEI (Checks-Effects-Interactions) pattern
    • State changes (_safeMint and _setTokenURI) occur before external calls
    • The onlyGateway modifier provides additional security by restricting access

Suggested improvements:

  • Replace hardcoded 700000 with gasLimit state variable in onCall function:
IZRC20(destination).withdrawGasFeeWithGasLimit(gasLimit)
...
CallOptions(gasLimit, false)
🔗 Analysis chain

Consider gas limit configuration and reentrancy protection.

While the onlyGateway modifier addition enhances security, there are two considerations:

  1. The function uses a hardcoded gas limit (700000) which differs from the contract's gasLimit state variable.
  2. The function performs external calls after state changes, which could be vulnerable to reentrancy.

Consider these improvements:

- gateway.call(
-     counterparty[destination],
-     destination,
-     abi.encode(tokenId, sender, uri),
-     CallOptions(700000, false),
-     RevertOptions(address(0), false, address(0), "", 0)
- );
+ // Use contract's configurable gas limit
+ gateway.call(
+     counterparty[destination],
+     destination,
+     abi.encode(tokenId, sender, uri),
+     CallOptions(gasLimit, false),
+     RevertOptions(address(0), false, address(0), "", 0)
+ );

Let's verify the external call patterns:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potential reentrancy patterns in the codebase
ast-grep --pattern 'function $FNAME($_) {
  $$$
  _safeMint($_, $_);
  $$$
  gateway.call($$$);
  $$$
}'

Length of output: 106

Script:

#!/bin/bash
# Let's search for the onCall function implementation to analyze state changes and external calls
ast-grep --pattern 'function onCall($_) {
  $$$
}'

# Also search for gateway.call patterns to understand the gas limit usage
rg "gateway\.call" -A 3

Length of output: 1445

Script:

#!/bin/bash
# Let's examine the full Universal.sol contract to understand the context better
cat examples/nft/contracts/Universal.sol

# Also check for gasLimit state variable usage
rg "gasLimit" -A 2 examples/nft/contracts/Universal.sol

Length of output: 6279

if (keccak256(context.origin) != keccak256(counterparty[zrc20]))
revert("Unauthorized");

Expand Down Expand Up @@ -140,7 +145,7 @@ contract Universal is
}
}

function onRevert(RevertContext calldata context) external {
function onRevert(RevertContext calldata context) external onlyGateway {
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add input validation for onRevert handler.

While the onlyGateway modifier provides access control, consider adding validation for the decoded parameters to prevent potential issues with malformed input data.

Add input validation:

 function onRevert(RevertContext calldata context) external onlyGateway {
     (uint256 tokenId, address sender, string memory uri) = abi.decode(
         context.revertMessage,
         (uint256, address, string)
     );
+    require(sender != address(0), "Invalid sender address");
+    require(bytes(uri).length > 0, "Empty URI");
 
     _safeMint(sender, tokenId);
     _setTokenURI(tokenId, uri);
 }

(uint256 tokenId, address sender, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
Expand Down
10 changes: 5 additions & 5 deletions examples/nft/scripts/test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,31 +45,31 @@ npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT
npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" --json &>/dev/null
npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null

nft_balance
npx hardhat localnet-check
nft_balance
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance visibility of NFT state transitions.

While the current placement of nft_balance calls provides good visibility of post-operation states, consider adding checks before operations to clearly show state transitions.

Consider this pattern for operations:

+# Before minting
+echo -e "\nInitial balances:"
+nft_balance
+
 NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId')
 echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain."
 
 npx hardhat localnet-check
+echo -e "\nBalances after minting:"
 nft_balance

Apply this pattern before and after each major operation (transfers) to provide clear visibility of state changes.

Also applies to: 55-55, 61-61, 67-67, 73-73


NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId')
echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain."

nft_balance
npx hardhat localnet-check
nft_balance

echo -e "\nTransferring NFT: ZetaChain → Ethereum..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"

nft_balance
npx hardhat localnet-check
nft_balance

echo -e "\nTransferring NFT: Ethereum → BNB..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1

nft_balance
npx hardhat localnet-check
nft_balance

echo -e "\nTransferring NFT: BNB → ZetaChain..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB"

nft_balance
npx hardhat localnet-check
nft_balance

npx hardhat localnet-stop
11 changes: 9 additions & 2 deletions examples/swap/contracts/Swap.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ contract Swap is UniversalContract {
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(address systemContractAddress, address payable gatewayAddress) {
systemContract = SystemContract(systemContractAddress);
gateway = GatewayZEVM(gatewayAddress);
Expand All @@ -31,7 +36,7 @@ contract Swap is UniversalContract {
address zrc20,
uint256 amount,
bytes calldata message
) external override {
) external override onlyGateway {
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding reentrancy protection.

While the onlyGateway modifier provides access control, the function performs multiple external calls during token operations. Consider adding a reentrancy guard to prevent potential attack vectors.

-    ) external override onlyGateway {
+    ) external override onlyGateway nonReentrant {

Add the ReentrancyGuard import and inheritance:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

contract Swap is UniversalContract, ReentrancyGuard {

Params memory params = Params({target: address(0), to: bytes("")});
if (context.chainID == BITCOIN) {
params.target = BytesHelperLib.bytesToAddress(message, 0);
Expand Down Expand Up @@ -105,5 +110,7 @@ contract Swap is UniversalContract {
);
}

function onRevert(RevertContext calldata revertContext) external override {}
function onRevert(
RevertContext calldata revertContext
) external override onlyGateway {}
}
11 changes: 9 additions & 2 deletions examples/swap/contracts/SwapToAnyToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ contract SwapToAnyToken is UniversalContract {
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;

modifier onlyGateway() {
require(msg.sender == address(gateway), "Caller is not the gateway");
_;
}

constructor(address systemContractAddress, address payable gatewayAddress) {
systemContract = SystemContract(systemContractAddress);
gateway = GatewayZEVM(gatewayAddress);
Expand All @@ -33,7 +38,7 @@ contract SwapToAnyToken is UniversalContract {
address zrc20,
uint256 amount,
bytes calldata message
) external virtual override {
) external virtual override onlyGateway {
Params memory params = Params({
target: address(0),
to: bytes(""),
Expand Down Expand Up @@ -147,5 +152,7 @@ contract SwapToAnyToken is UniversalContract {
swapAndWithdraw(inputToken, amount, targetToken, recipient, withdraw);
}

function onRevert(RevertContext calldata revertContext) external override {}
function onRevert(
RevertContext calldata revertContext
) external override onlyGateway {}
}
Loading