Fix named error parameter encoding order in IR codegen by nikola-matic · Pull Request #16453 · argotorg/solidity

nikola-matic · 2026-02-09T09:55:59Z

cameel

This needs a lot more tests and a bug list entry.

cameel · 2026-03-16T19:15:57Z

As a part of due diligence for the fix we must make sure that similar functionality in other places is not affected. In this case this means checking named parameter support in language constructs. I'd especially verify events, as these share a lot of code with errors, but are more:

emitting events

function calls

internal

external

library

bound

virtual

Especially when the names in the base function are in a different order.

getter

bare calls

struct constructor invocations

https://github.com/argotorg/solidity/blob/develop/test/libsolidity/semanticTests/structs/struct_named_constructor.sol

For these named parameters are not allowed, but they should be still covered with (failing) tests using them:

builtin calls

ABI encoding builtins

inherited constructor calls

modifier invocations

calls via function pointers

type conversions (these are not calls, but look the same to the parser)

Is there anything else I'm missing?

A couple of questions:

When you say getters - do you mean auto generated ones, and how would named parameters fit here? My assumption is that getters don't take any parameters?

What do you mean by bare calls? call, staticcall, delegatecall? Can you give an example?

My assumption is that getters don't take any parameters?

They do on array types. Though, now that I think of it, I'm not sure we support named arguments there. Please check in any case.

What do you mean by bare calls? call, staticcall, delegatecall?

Yes. Not sure what the name of the data argument is (and if we actually support named arguments there) but it would be something like this:

a.call({data: ""});

cameel · 2026-04-02T10:13:44Z

@@ -1,4 +1,17 @@
 [
+    {
+        "uid": "SOL-2026-2",


Just a heads-up to bump this in case we merge #16508 first.

And another heads-up for the date in blog post link.

cameel · 2026-04-02T10:15:12Z

 [
+    {
+        "uid": "SOL-2026-2",
+        "name": "CustomErrorNamedParametersEncodedInWrongOrder",


How about this?

Suggested change

"name": "CustomErrorNamedParametersEncodedInWrongOrder",

"name": "MisorderedNamedParametersInRequireWithCustomErrors",

cameel · 2026-04-02T11:01:25Z

 			auto const& errorConstructorCall = dynamic_cast<FunctionCall const&>(*arguments[1]);
 			appendCode() << m_utils.requireWithErrorFunction(errorConstructorCall) << "(" <<IRVariable(*arguments[0]).name();
-			for (auto argument: errorConstructorCall.arguments())
+			for (auto argument: errorConstructorCall.sortedArguments())


I think that there are still some things we can do to make these errors less likely without going for full #16529. One of them is to always use sortedArguments() by default.

We have very few places where we need arguments in call order. I looked through the code and in almost all cases where arguments() is invoked, sortedArguments() would both would work, either because the function has fewer than two arguments or because it's a case where we don't allow named arguments. We should change all of them to sortedArguments(). This may prevent more bugs creeping in when we extend named parameter support to more places. People will also be less likely to choose arguments() just because the surrounding code uses it.

I would also rename them to argumentsInDefinitionOrder() and argumentsInCallOrder(). The current naming makes it seem like the arguments() is the default one and sortedArguments() is something special. The docstring should also have a big fat warning explaining the difference.

This, along with the extra tests for named parameters could be extracted into a separate PR and merged quicker.

cameel · 2026-04-02T11:28:13Z

+        revert NamedArgsError3({c: 9, a: 2, b: 7});
+    }
+    function trigger4() external pure {
+        revert NamedArgsError4({b: "error", a: 2, c: 9, d: true});


Wait, I thought mismatched types would produce an error without the fix, but I just checked this example on 0.8.34 and it still compiles. Makes sense given that the error is in the codegen, not in analysis, but this makes the bug worse than we assumed based on the information we had when we discussed it. Why didn't you say anything about it?

When we discussed this, the assumption was that in most cases bad code would not even compile due to type mismatch and you could at most swap two numbers or something. If any combination compiles, it's much easier to run into the bug.

The consequences are also a bit different. Now you can have errors whose encoded parameters do not match the selector. Depending on how the decoder works, it may result in a decoding failure, which is at least better than accepting broken data (but still worse than a compilation error).

cameel · 2026-04-02T11:37:58Z

+        revert NamedArgsError3({c: 9, a: 2, b: 7});
+    }
+    function trigger4() external pure {
+        revert NamedArgsError4({b: "error", a: 2, c: 9, d: true});


One more thing we need coverage for: implicit conversions. I assumed that the parameters were misordered but at least type-checked correctly to ensure they still match the position at which they are used. This is not the case so we have to consider what that can break.

For example, consider strings. These can take different number of slots depending on where they are stored (calldata, storage, memory, nowhere (literals)) and the conversion is the place where we account for that. Does this mean that the misordering could result in the wrong number of item being taken off the stack? If so, that could affect control flow. The fact that this is a part of a revert limits the blast radius, but it might even result in a revert not happening if it's bad enough.

cameel · 2026-04-02T11:40:19Z

+        "summary": "Custom error arguments passed using named parameters are ABI-encoded in call order instead of declaration order when compiling via IR.",
+        "description": "When a custom error is instantiated using named parameters (e.g., `require(cond, MyError({b: 1, a: 2}))`), the IR-based code generator incorrectly encodes the arguments in the order they appear at the call site rather than in the order the parameters are declared in the error definition. This results in silently wrong ABI-encoded revert data, swapping argument values relative to their declared positions. The legacy pipeline is not affected.",


This is missing some key information:

That revert is not affected (would not hurt to mention that other uses of named parameters are safe too)

What happens when argument types don't match (I assumed it would be a compilation error but actually it seems that it's worse)

legacy pipeline

evmasm pipeline

github-actions · 2026-04-16T12:25:00Z

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

cameel reviewed Feb 9, 2026

View reviewed changes

Comment thread libsolidity/codegen/ir/IRGeneratorForStatements.cpp

cameel changed the title ~~Fix named error paremeter encoding order in IR codegen~~ Fix named error parameter encoding order in IR codegen Feb 9, 2026

github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Feb 24, 2026

nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Feb 24, 2026

github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 11, 2026

argotorg deleted a comment from github-actions Bot Mar 11, 2026

nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 11, 2026

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch 3 times, most recently from 01fe6b9 to 0f978ea Compare March 13, 2026 15:34

cameel requested changes Mar 16, 2026

View reviewed changes

cameel added the codegen error Compiler generates invalid code. Critical. label Mar 16, 2026

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch 5 times, most recently from a6503ed to 8bd6e00 Compare March 17, 2026 13:15

nikola-matic mentioned this pull request Mar 17, 2026

Refactor the mechanism for supplying arguments in codegen #16529

Open

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch from 8bd6e00 to 071ea8a Compare March 18, 2026 08:04

nikola-matic requested a review from cameel March 18, 2026 08:05

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch from 071ea8a to 4524716 Compare March 18, 2026 12:00

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch 2 times, most recently from 6c95a91 to 04f0ce0 Compare March 25, 2026 12:47

cameel requested changes Apr 2, 2026

View reviewed changes

github-actions Bot added the stale The issue/PR was marked as stale because it has been open for too long. label Apr 16, 2026

nikola-matic removed the stale The issue/PR was marked as stale because it has been open for too long. label Apr 16, 2026

cameel added this to the 0.8.35 milestone Apr 18, 2026

Fix named error paremeter encoding order in IR codegen

0ed1b17

nikola-matic added 2 commits April 21, 2026 12:25

Additional test coverage for named parameters

fd41ec7

Buglist entry

3529a21

nikola-matic force-pushed the fix-named-error-paramater-encoding-in-ir branch from 04f0ce0 to 3529a21 Compare April 21, 2026 10:36

	"name": "CustomErrorNamedParametersEncodedInWrongOrder",
	"name": "MisorderedNamedParametersInRequireWithCustomErrors",

		"summary": "Custom error arguments passed using named parameters are ABI-encoded in call order instead of declaration order when compiling via IR.",
		"description": "When a custom error is instantiated using named parameters (e.g., `require(cond, MyError({b: 1, a: 2}))`), the IR-based code generator incorrectly encodes the arguments in the order they appear at the call site rather than in the order the parameters are declared in the error definition. This results in silently wrong ABI-encoded revert data, swapping argument values relative to their declared positions. The legacy pipeline is not affected.",

Conversation

nikola-matic commented Feb 9, 2026

Uh oh!

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

cameel Mar 16, 2026 • edited by nikola-matic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cameel Mar 16, 2026 •

edited by nikola-matic

Loading

cameel Mar 17, 2026 •

edited

Loading