EVM: make transferEntryPoint virtual

[0xIryna]

Overview

This PR makes _transferEntryPoint function virtual in NttManager contract allowing more flexibility for integrators

Changes

• Added virtual modifier to _transferEntryPoint function
• No changes to the existing function logic or behavior

Motivation

• Ability to add custom logic
• Ability to remove unused code and save gas

Impact

• No breaking changes
• Backwards compatible
• No security implications as core logic remains unchanged

Testing

• Existing tests remain unchanged and passing

[djb15]

This in itself is safe, but it pushes a lot of burden to the integrator and should not be overridden lightly.

Is the integrator problem something that could be solved with a new hook instead?

[0xIryna]

@djb15 thank you for your feedback!
Our use-case is pretty unique. Here are the issues we run into:

• we want to extend our NTTManager with the ability to transfer, not just the default token, but also a wrapped version of this token. The default NTTManager implementation supports only one token, and we need to duplicate the whole transfer functionality to support a wrapped version.
• we run into Yul stack too deep issue see CI output after modifying custom event in _prepareNativeTokenTransfer which is tricky to fix if we don't have control over all the code in transfer function.
• high gas cost of transfer function. By overriding _transferEntryPoint function we were able to reduce the gas cost of transfer by 4000-5000 gas.

It might be possible to solve all these issues with additional hooks, but it will require more intrusive changes to NTTNanager which won't be applicable to the majority of integrators.

[0xIryna]

@djb15 do you have any recommendation on fixing stack too deep issues when compiling with via-ir=true?
We upgraded our repo to use the latest native-token-transfers code and now the compilation is failing.
Steps to reproduce:

1. Checkout upgrade-ntt-manager branch from git@github.com:m0-foundation/m-portal.git
2. run FOUNDRY_PROFILE=production forge build

Observe

[⠑] Solc 0.8.26 finished in 7.59s
Error: Compiler run failed:
Error: Yul exception:Variable expr_59838_mpos is 1 too deep in the stack [ RET var_sequence expr_59838_mpos size expr_component var_recipient expr_1 var_recipientChain var_amount expr_2 _13 expr_component_mpos expr_mpos ret_3 memPtr cleaned var_i _14 _mpos ]
memoryguard was present.
YulException: Variable expr_59838_mpos is 1 too deep in the stack [ RET var_sequence expr_59838_mpos size expr_component var_recipient expr_1 var_recipientChain var_amount expr_2 _13 expr_component_mpos expr_mpos ret_3 memPtr cleaned var_i _14 _mpos ]
memoryguard was present.

Looks like this commit from native-token-transfers makes our code fail. Removing the indexed attribute fixes the issue, but it's not an acceptable solution and we cannot use via-ir=false due to NTTManager contract size issues.

[djb15]

@0xIryna thanks for the context! No objections to making those changes on a fork, but is there a reason you want to get this into main here? @nik-suri I don't know if we want it to be easy for anyone to override this entrypoint unless you really really know what you're doing and do so in your own fork. My preference from a security point of view would be not to expose this function as virtual for all integrators, but if there's a strong case across multiple integrators we merge this in; like I mentioned above, doing this isn't bad in itself, it just introduces risk when overridden.

Regarding the stack issues, I believe that's specific to your fork as I'm not able to replicate on the tip of main here. My general advise for trying to resolve those errors is trying to block scope variables. I feel your pain though, it's annoyingly easy to hit the EVM stack constraints with a moderately complex function

[0xIryna]

@djb15 Thank you for your feedback! I understand your concern, we will use the fork then.

As for stack too deep. The issue is related to via-ir compilation and cannot be easily solved by adding block scope variables. There are a bunch of similar issues in solidity repo (14358, 14378, 14187, etc.) and there is no solution that fits all. The current default NTTManager implementation doesn't have the issue, and our codebase didn't have it until we upgraded to the latest NTT code, so it's a combination of factors that causes it. Any other integrator might encounter it since we're forced to use via-ir

[djb15]

Yeah sadly the --via-ir build flag is necessary right now as we're cutting it tight with the contact size for the default implementation. Thanks for flagging again though, it's something I know the other core contributors are thinking about to improve the integrator experience. But like you say I don't think there's a generic fix for this yet, it's entirely application specific