Skip to content

fix: eth_sendRawTransaction now handles malformed RLP data correctly #3765

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: eth_sendRawTransaction now handles malformed RLP data correctly #3765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b34292f
fix: added check for bad RLP hex
simzzz May 15, 2025
c59b247
fix: undo unnecessary changes
simzzz May 15, 2025
a7ec395
limit change of error handling for only bad RLP hexes to avoid unwant…
simzzz May 16, 2025
fd5e791
change error code in test
simzzz May 16, 2025
e600d1f
changed error message in tests
simzzz May 16, 2025
5edc18b
fixed failing test
simzzz May 16, 2025
4b26464
fixed failing test
simzzz May 16, 2025
2e41c24
fixed another error message
simzzz May 16, 2025
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 packages/relay/src/lib/precheck.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,14 @@ export class Precheck {
* @returns {Transaction} The parsed transaction.
*/
public static parseRawTransaction(transaction: string | Transaction): Transaction {
return typeof transaction === 'string' ? Transaction.from(transaction) : transaction;
try {
return typeof transaction === 'string' ? Transaction.from(transaction) : transaction;
} catch (e: any) {
if (e.message.toString().includes('unexpected junk after rlp payload')) {
throw predefined.INVALID_ARGUMENTS(e.message.toString());
}
throw predefined.INTERNAL_ERROR(e.message.toString());
}
}
Comment on lines +40 to 46
Copy link
Contributor

Choose a reason for hiding this comment

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

I’d suggest removing this overhead and simply returning INVALID_ARGUMENTS. At this point, the only operation that throws an error is the Transaction.from(), which functions as a parsing engine. If it fails, it indicates an issue with the transaction hash, which is the argument itself—so I believe there’s no need to use INTERNAL_ERROR here.


/**
Expand Down
13 changes: 13 additions & 0 deletions packages/relay/tests/lib/eth/eth_sendRawTransaction.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,19 @@ describe('@ethSendRawTransaction eth_sendRawTransaction spec', async function ()
);
});

it('should return a predefined INVALID_ARGUMENTS when transaction has invalid format', async function () {
// signature has been truncated
const invalidTx =
'0xf8748201280585800e8dfc0085800e8dfc00832dc6c094aca85ef7e1fce27079bbf99b60fcf6fd19b99b248502540be40080c001a0210446cfb671c3174392410d52fa3cd58723d8417e40cc67c6225b8f7e3ff693a02674b392846c59f783ea96655d39560956dd987051972064a5d853cea0b6f6d711';
await RelayAssertions.assertRejection(
predefined.INVALID_ARGUMENTS('invalid hex string'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I wanna see the unexpected junk after rlp payload to be covered since this PR is aiming to fix that, right? How about we use the provided signed transaction for this?

ethImpl.sendRawTransaction,
false,
ethImpl,
[invalidTx, requestDetails],
);
});

it('should return a computed hash if unable to retrieve EthereumHash from record due to contract revert', async function () {
const signed = await signTransaction(transaction);

Expand Down
15 changes: 14 additions & 1 deletion packages/relay/tests/lib/precheck.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ describe('Precheck', async function () {
const txWithZeroValue =
'0xf86380843b9aca00825208940000000000000000000000000000000000000000808025a04e557f2008ff383df9a21919860939f60f4c27b9c845b89021ae2a79be4f6790a002f86d6dcefd2ffec72bf4d427091e7375acb6707e49d99893173cbc03515fd6';
const parsedTxWithZeroValue = ethers.Transaction.from(txWithZeroValue);

const invalidTx =
'6080604052348015600f57600080fd5b50603f80601d6000396000f3fe6080604052600080fdfea26469706673582212209c06253b6069b4e1f720945c020dc1c7b3d74b850eba35ac8b6fb407eff7ca7364736f6c63430008120033';
const defaultGasPrice = 720_000_000_000;
const defaultGasLimit = 1_000_000;
const defaultChainId = Number('0x12a');
Expand Down Expand Up @@ -943,4 +944,16 @@ describe('Precheck', async function () {
expect(async () => await precheck.receiverAccount(parsedTx, requestDetails)).not.to.throw;
});
});

describe('parseRawTransaction', async function () {
it('should successfully parse a valid transaction string', function () {
const parsedTx = Precheck.parseRawTransaction(parsedTxWithMatchingChainId);
expect(parsedTx).to.be.instanceOf(Transaction);
expect(parsedTx.chainId).to.equal(BigInt(298));
});

it('should throw INVALID_ARGUMENTS for invalid RLP', function () {
expect(() => Precheck.parseRawTransaction(invalidTx)).to.throw('Error invoking RPC: invalid BytesLike value');
});
Comment on lines +956 to +957
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's test the unexpected junk after RLP payload issue.

});
});
14 changes: 13 additions & 1 deletion packages/server/tests/acceptance/rpc_batch1.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1002,7 +1002,7 @@ describe('@api-batch-1 RPC Server Acceptance Tests', function () {
};

const signedTx = await accounts[2].wallet.signTransaction(transaction);
const error = predefined.INTERNAL_ERROR();
const error = predefined.INVALID_ARGUMENTS('unexpected junk after rlp payload');

await Assertions.assertPredefinedRpcError(error, sendRawTransaction, false, relay, [
signedTx + '11',
Expand Down Expand Up @@ -1535,6 +1535,18 @@ describe('@api-batch-1 RPC Server Acceptance Tests', function () {
]);
});

it('should fail "eth_sendRawTransaction" when transaction has invalid format', async function () {
// signature has been truncated
const invalidTx =
'0xf8748201280585800e8dfc0085800e8dfc00832dc6c094aca85ef7e1fce27079bbf99b60fcf6fd19b99b248502540be40080c001a0210446cfb671c3174392410d52fa3cd58723d8417e40cc67c6225b8f7e3ff693a02674b392846c59f783ea96655d39560956dd987051972064a5d853cea0b6f6d711';
const error = predefined.INVALID_ARGUMENTS('unexpected junk after rlp payload');

await Assertions.assertPredefinedRpcError(error, sendRawTransaction, true, relay, [
invalidTx,
requestDetails,
]);
});

it('should fail "eth_sendRawTransaction" for EIP155 transaction with a too high gasLimit', async function () {
const gasLimit = 999999999;
const transaction = {
Expand Down
29 changes: 25 additions & 4 deletions packages/ws-server/tests/acceptance/sendRawTransaction.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,8 @@ describe('@web-socket-batch-2 eth_sendRawTransaction', async function () {
const requestId = 'sendRawTransactionTest_ws-server';
const requestDetails = new RequestDetails({ requestId: requestId, ipAddress: '0.0.0.0' });

let tx: any,
sendHbarToProxyContractDeployerTx: any,
accounts: AliasAccount[] = [],
ethersWsProvider: WebSocketProvider;
let tx: any, sendHbarToProxyContractDeployerTx: any, ethersWsProvider: WebSocketProvider;
const accounts: AliasAccount[] = [];

before(async () => {
const initialAccount: AliasAccount = global.accounts[0];
Expand Down Expand Up @@ -112,6 +110,16 @@ describe('@web-socket-batch-2 eth_sendRawTransaction', async function () {
expect(fromAccountInfo.evm_address).to.eq(accounts[0].address.toLowerCase());
});

it(`Should fail eth_sendRawTransaction on Standard Web Socket when transaction has invalid format`, async () => {
// signature has been truncated
const invalidTx =
'0xf8748201280585800e8dfc0085800e8dfc00832dc6c094aca85ef7e1fce27079bbf99b60fcf6fd19b99b248502540be40080c001a0210446cfb671c3174392410d52fa3cd58723d8417e40cc67c6225b8f7e3ff693a02674b392846c59f783ea96655d39560956dd987051972064a5d853cea0b6f6d711';
const response = await WsTestHelper.sendRequestToStandardWebSocket(METHOD_NAME, [invalidTx], 1000);

expect(response.error.code).to.eq(-32603);
expect(response.error.message).to.contain('unexpected junk after rlp payload');
});

it(`Should execute eth_sendRawTransaction on Standard Web Socket for the deterministic deployment transaction`, async () => {
// send gas money to the proxy deployer
sendHbarToProxyContractDeployerTx.nonce = await global.relay.getAccountNonce(accounts[0].address);
Expand Down Expand Up @@ -178,6 +186,19 @@ describe('@web-socket-batch-2 eth_sendRawTransaction', async function () {
expect(fromAccountInfo.evm_address).to.eq(accounts[1].address.toLowerCase());
});

it(`Should fail eth_sendRawTransaction on Ethers Web Socket Provider when transaction has invalid format`, async () => {
// signature has been truncated
const invalidTx =
'0xf8748201280585800e8dfc0085800e8dfc00832dc6c094aca85ef7e1fce27079bbf99b60fcf6fd19b99b248502540be40080c001a0210446cfb671c3174392410d52fa3cd58723d8417e40cc67c6225b8f7e3ff693a02674b392846c59f783ea96655d39560956dd987051972064a5d853cea0b6f6d711';
try {
await ethersWsProvider.send(METHOD_NAME, [invalidTx]);
expect.fail('Should have thrown an error');
} catch ({ error }) {
expect(error.code).to.eq(-32603);
expect(error.message).to.contain('unexpected junk after rlp payload');
}
});

it(`Should execute eth_sendRawTransaction on Ethers Web Socket Provider for the deterministic deployment transaction`, async () => {
// send gas money to the proxy deployer
sendHbarToProxyContractDeployerTx.nonce = await global.relay.getAccountNonce(accounts[1].address);
Expand Down
Loading