From cc76b6e2184f6615705ff60b78f4ed6943eaafd3 Mon Sep 17 00:00:00 2001 From: bars26 Date: Tue, 15 Sep 2026 23:38:34 +0300 Subject: [PATCH] fix: ConsensusMain.addTransaction trusts caller-supplied _sender over the verified signer `addTransaction`'s `_sender` argument was accepted from the caller with no check against who actually authorized the transaction, letting anyone submit a call that GenVM attributes to an arbitrary victim address: - Solidity: `ConsensusMain._addTransaction` / `ConsensusMainWithFees._addTransaction` only defaulted `_sender` to `msg.sender` when it was `address(0)`, but otherwise accepted whatever the caller passed. The external `addTransaction` entry points now reject `_sender != msg.sender` unless the caller is a registered ghost contract relaying its own caller (the legitimate case used by `GhostBlueprint.addTransaction`). - Python: `TransactionParser.get_genlayer_transaction` used `rollup_transaction.data.args.sender` (decoded straight from calldata) as the Intelligent Contract caller, instead of `rollup_transaction.from_address` (the signer already verified by `transaction_has_valid_signature`). It now rejects the transaction when the two disagree. Every auth check that trusts `gl.message.sender_address` (token gates, owner-only methods, per-user balances) was bypassable through this gap; the real signer only paid the gas. Fixes genlayerlabs/genlayer-studio#1768. Co-Authored-By: Claude Sonnet 5 --- backend/protocol_rpc/transactions_parser.py | 16 +++++ .../contracts/v2_contracts/ConsensusMain.sol | 43 +++++++++---- .../v2_contracts/ConsensusMainWithFees.sol | 12 ++++ .../contracts/v2_contracts/utils/Errors.sol | 1 + tests/unit/test_transactions_parser.py | 61 ++++++++++++++++++- 5 files changed, 121 insertions(+), 12 deletions(-) diff --git a/backend/protocol_rpc/transactions_parser.py b/backend/protocol_rpc/transactions_parser.py index f2c4dbc73..b006d1edc 100644 --- a/backend/protocol_rpc/transactions_parser.py +++ b/backend/protocol_rpc/transactions_parser.py @@ -13,6 +13,7 @@ from backend.rollup.consensus_service import ConsensusService from backend.rollup.web3_pool import Web3ConnectionPool from backend.domain.types import TransactionType +from backend.errors.errors import InvalidTransactionError from backend.protocol_rpc.types import ( DecodedDeploymentData, @@ -597,6 +598,21 @@ def get_genlayer_transaction( ) sender = rollup_transaction.data.args.sender + # `sender` above is an attacker-controlled argument decoded from the + # transaction's calldata, not the party that actually authorized the + # transaction. Only `rollup_transaction.from_address` is verified, + # by `transaction_has_valid_signature`, to be the recovered signer + # of the raw transaction. Without this check, a caller can sign a + # valid transaction from their own address while setting `sender` + # to any other address, which becomes the Intelligent Contract + # caller (see GenVM's `gl.message.sender_address`) while the real + # signer merely pays the gas. + if to_checksum_address(sender) != to_checksum_address( + rollup_transaction.from_address + ): + raise InvalidTransactionError( + "Transaction sender argument does not match the recovered signer" + ) # The v0.6 deploySalted selector is authoritative: Consensus ignores # the tuple recipient and always passes address(0) into CreationPhase. # Inferring only from the user-controlled recipient let a salted deploy diff --git a/hardhat/contracts/v2_contracts/ConsensusMain.sol b/hardhat/contracts/v2_contracts/ConsensusMain.sol index f80a57608..276c87816 100644 --- a/hardhat/contracts/v2_contracts/ConsensusMain.sol +++ b/hardhat/contracts/v2_contracts/ConsensusMain.sol @@ -303,18 +303,29 @@ contract ConsensusMain is uint256 _maxRotations, bytes memory _txData ) external { - if (_txData.length > 0) { - _addTransaction( - _sender, - _recipient, - _numOfInitialValidators, - _maxRotations, - 0, - _txData - ); - } else { + if (_txData.length == 0) { revert Errors.EmptyTransaction(); } + // A caller may only submit a transaction on behalf of another + // address if it is a registered ghost contract relaying a call for + // its own caller (see GhostBlueprint.addTransaction). Otherwise + // `_sender` must be the caller itself, or left unset (address(0), + // defaulted to msg.sender below). + if ( + _sender != address(0) && + _sender != msg.sender && + !ghostContracts[msg.sender] + ) { + revert Errors.UnauthorizedSender(); + } + _addTransaction( + _sender, + _recipient, + _numOfInitialValidators, + _maxRotations, + 0, + _txData + ); // TODO: Fee verification handling } @@ -396,8 +407,18 @@ contract ConsensusMain is } else { saltNonce = 0; } + address feeAwareSender = address(uint160(senderWord)); + // Same authorization rule as the plain `addTransaction` entrypoint: + // see the comment there for why `_sender` cannot be trusted as-is. + if ( + feeAwareSender != address(0) && + feeAwareSender != msg.sender && + !ghostContracts[msg.sender] + ) { + revert Errors.UnauthorizedSender(); + } _addTransaction( - address(uint160(senderWord)), + feeAwareSender, address(uint160(recipientWord)), numOfInitialValidators, maxRotations, diff --git a/hardhat/contracts/v2_contracts/ConsensusMainWithFees.sol b/hardhat/contracts/v2_contracts/ConsensusMainWithFees.sol index 2e6191320..0e6d8b775 100644 --- a/hardhat/contracts/v2_contracts/ConsensusMainWithFees.sol +++ b/hardhat/contracts/v2_contracts/ConsensusMainWithFees.sol @@ -75,6 +75,18 @@ contract ConsensusMainWithFees is if (_txData.length == 0) { revert Errors.EmptyTransaction(); } + // A caller may only submit a transaction on behalf of another + // address if it is a registered ghost contract relaying a call for + // its own caller (see GhostBlueprint.addTransaction). Otherwise + // `_sender` must be the caller itself, or left unset (address(0), + // defaulted to msg.sender below). + if ( + _sender != address(0) && + _sender != msg.sender && + !ghostContracts[msg.sender] + ) { + revert Errors.UnauthorizedSender(); + } uint totalFeesToPay = feeManager.calculateRoundFees( bytes32(0), _feesDistribution, diff --git a/hardhat/contracts/v2_contracts/utils/Errors.sol b/hardhat/contracts/v2_contracts/utils/Errors.sol index a79cb9d29..b451a42c5 100644 --- a/hardhat/contracts/v2_contracts/utils/Errors.sol +++ b/hardhat/contracts/v2_contracts/utils/Errors.sol @@ -85,4 +85,5 @@ library Errors { error CallerNotModule(); error CallerNotOwner(); error InvalidProcessingBlock(); + error UnauthorizedSender(); } \ No newline at end of file diff --git a/tests/unit/test_transactions_parser.py b/tests/unit/test_transactions_parser.py index baec83f6a..1050cc374 100644 --- a/tests/unit/test_transactions_parser.py +++ b/tests/unit/test_transactions_parser.py @@ -2,6 +2,8 @@ from unittest.mock import Mock, MagicMock from backend.database_handler.transactions_processor import TransactionsProcessor from backend.database_handler.models import TransactionStatus +from backend.domain.types import TransactionType +from backend.errors.errors import InvalidTransactionError from backend.protocol_rpc.transactions_parser import ( TransactionParser, DecodedMethodSendData, @@ -13,7 +15,6 @@ DecodedRollupTransactionDataArgs, ZERO_ADDRESS, ) -from backend.domain.types import TransactionType import re from typing import Optional, List, Any from rlp import encode @@ -1070,3 +1071,61 @@ def test_pre_fee_submission_decodes_without_a_rollup_connection(monkeypatch): genlayer_transaction = parser.get_genlayer_transaction(decoded) assert genlayer_transaction.type == TransactionType.DEPLOY_CONTRACT assert genlayer_transaction.data.contract_code == contract_code + + +def _rollup_transaction_with_sender( + *, from_address: str, args_sender: str +) -> DecodedRollupTransaction: + return DecodedRollupTransaction( + from_address=from_address, + to_address="0x6666666666666666666666666666666666666666", + data=DecodedRollupTransactionData( + function_name="addTransaction", + args=DecodedRollupTransactionDataArgs( + sender=args_sender, + recipient="0x6666666666666666666666666666666666666666", + num_of_initial_validators=5, + max_rotations=3, + data=encode([b"\x00"]), + ), + ), + type=2, + nonce=1, + value=0, + ) + + +def test_get_genlayer_transaction_uses_recovered_signer_when_sender_matches( + transaction_parser, +): + # Legitimate case: the caller signed for themselves, so the `sender` + # argument and the recovered signer agree. + rollup_transaction = _rollup_transaction_with_sender( + from_address="0x7777777777777777777777777777777777777777", + args_sender="0x7777777777777777777777777777777777777777", + ) + + genlayer_transaction = transaction_parser.get_genlayer_transaction( + rollup_transaction + ) + + assert ( + genlayer_transaction.from_address + == "0x7777777777777777777777777777777777777777" + ) + assert genlayer_transaction.type == TransactionType.RUN_CONTRACT + + +def test_get_genlayer_transaction_rejects_sender_spoofing(transaction_parser): + # Attacker signs the transaction from their own address (from_address) + # but sets the `sender` calldata argument to a victim address. Trusting + # `sender` here would let the attacker impersonate the victim as the + # Intelligent Contract caller while the victim never authorized + # anything - the real signer merely pays the gas. + rollup_transaction = _rollup_transaction_with_sender( + from_address="0x8888888888888888888888888888888888888888", # attacker + args_sender="0x9999999999999999999999999999999999999999", # victim + ) + + with pytest.raises(InvalidTransactionError): + transaction_parser.get_genlayer_transaction(rollup_transaction)