Skip to content
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.

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

Removal of x/authz dependency on precompiles #47

Open
vladjdk opened this issue Apr 3, 2025 · 6 comments · May be fixed by #62
Open

Removal of x/authz dependency on precompiles #47

vladjdk opened this issue Apr 3, 2025 · 6 comments · May be fixed by #62
Assignees

Comments

@vladjdk
Copy link
Member

vladjdk commented Apr 3, 2025

Removal of x/authz dependency on precompiles

Summary

For erc20 precompile

  • Add Allowance state and CRUD methods in x/erc20 module.
  • Replace the parts of the ERC20 precompile that read/write allowances through authzKeeper with the use of erc20Keeper’s Allowance CRUD methods.

For other precompiles

Problems

1. Modularity of precompiles

  • Cosmos SDK and IBC-go modules such as bank, staking, and ibctransfer do not have a dependency on the authz module by default. However, the precompiles that are implemented to use these modules currently depend on the authz module.
  • By applying a modular structure that separates dependencies, chains that do not use the authz module can avoid having to include it just to use cosmos/evm.

2. Compatibility between ERC20 and authz spec

  • The Authz Grant data structure used to store permissions in the authz module includes an Expiration field, whereas allowances in the ERC20 spec do not. When approve is called through ERC20 precompile, Expiration is set to 1 year by default. However, When it is called thruogh x/auth module, it could be different value.
  • The SendAuthorization type from the x/bank module, which is used for storing ERC20 allowances, includes an AllowList field—a recipient address whitelist—which also does not exist in the ERC20 specification. When approve is called through the ERC20 precompile, AllowList is set to empty list that means all addresses can be recipient address, by default. However, if someone directly creates a SendAuthorization through the authz module, those fields can be used. This may lead to cases where an allowance exists, but transferFrom fails for certain addresses, or existing allowances disappear without any Transfer or Approval event being emitted.

3. Unconfigurable Expiration of authz Grant

  • When using authz functionality via precompile, the expiration time is currently hardcoded to one year. [ref: NewPrecompile]
  • This differs from using the actual authz module directly, where the expiration can be configured.
  • Therefore, even outside of the ERC20 module, there is a possibility of inconsistency between the Expiration of an authz Grant created via a precompile and that of a Grant created through the corresponding Cosmos SDK message.

Solutions

For erc20 precompile

  • Since the ERC20 spec includes approval functionality as it is, the ERC20 precompile should also have approval functionality. It is differenent from other precompiles whose corresponding sdk module doesn’t have approval funtionality.
  • Simply removing the authz dependency is not sufficient, as approve, allowance, and transferFrom would no longer function.
  • Thus, the ERC20 precompile requires its own allowance state.
  • We propose storing the ERC20 state in the x/erc20 module with the following key/value structure:
  • Key: AllowancePrefix([]byte{4}) + TokenPairID + ownerAddr + spenderAddr
  • Value: sdkmath.LegacyDec

For other modules

  • Currently, the precompiles using authz functionality are the ERC20, ICS20, and staking precompiles.
  • Among them, we propose removing methods that depend on the authz module from ICS20 and staking precompiles.
  • Since the common Precompile struct currently embeds authzKeeper as a field, that field should also be removed.
@dongsam dongsam assigned dongsam and cloudgray and unassigned dongsam Apr 4, 2025
@dongsam dongsam moved this to In Progress in cosmos/evm Phase 1 Apr 4, 2025
@vladjdk
Copy link
Member Author

vladjdk commented Apr 4, 2025

Hey, this generally looks good to me. To address each point individually:

  1. ERC20 Precompile: I agree on the direction that you're taking and we should follow the ERC20 behaviour 1:1 wherever possible, and allowance is no exception. The CRUD direction sounds reasonable.
  2. For other precompiles: I have no idea why there was even authz in these to begin with. The general direction for this seems reasonable, assuming that we take the necessary precautions for permissions gating via checking the signature and making sure that users can only perform changes on their own account.

@cloudgray
Copy link

@vladjdk Thanks for your feedback.

As for my speculation regarding the x/authz dependency in other precompiles:

  • In the case of modules like x/bank, x/staking, and ibctransfer, there are dedicated types that implement the authz.Authorization interface. [ref1][ref2][ref3] I think the intention was to provide precompile functionality that maps 1:1 with the grantable operations available through the Cosmos SDK using those types.

  • However, in trying to unify everything under a single authz interface, [ref4] compatibility issues may have arisen between the SDK modules and the precompiles — which poses a potential risk.

  • The reason the common Precompile struct includes an authzKeeper dependency might be due to the fact that the x/authz module can grant [ref5] and execute general messages [ref6] for any other module, so this was likely added with future extensibility in mind.

@cloudgray
Copy link

Internal Discussion Summary

TL;DR

  • We initially planned to remove the authz dependency entirely.
  • We revisited whether completely removing authz was the right decision after finding several contract-level use cases in Evmos’s original codebase. We considered introducing a new “authz precompile” to preserve those capabilities.
  • After discussion, we decided not to implement a new authz precompile and to continue with our original plan.

1. Background

Our original roadmap included removing the authz dependency from all precompiles. However, we noticed that existing Evmos examples rely on authz to let contracts act on behalf of end-user wallets (EoAs). This sparked an internal discussion on whether we should maintain these capabilities through a new authz precompile.

2. Use Cases and Examples

Evmos provided several examples—such as simple-staker, simple-ibc-transfer, and flashloan—where a contract can be granted permission (via authz) to manage an EoA’s assets. In these flows (EoA -> bizContract -> precompile), the contract requires authz approval to perform actions like staking (Delegate) or transferring tokens (IBC Transfer) on behalf of the user.

3. Considering New Authz Precompile

To preserve these use cases while still removing authz dependencies, we considered creating a dedicated authz precompile package that chains could optionally include. This would allow developers who need this functionality to continue leveraging authz without forcing it on every chain.

4. Conclusion: No Separate Authz Precompile

After evaluating the potential security implications, we decided not to move forward with a separate authz precompile. Reasons include:

  • Security Risks: The authz module lets contracts receive broad permissions to execute any SDK message on a user’s behalf. In a smart contract context, this could expose users to malicious contracts, where they might unknowingly grant permissions leading to unintended actions.
  • Alternative Approaches: In many cases, contracts can operate with directly transferred assets from users, eliminating the need for contract-level authz. This approach is common in EVM-based systems, where authorization is managed through each contract’s internal state. For example, a liquid staking contract typically requires the user to send tokens to the contract first, rather than staking the user’s tokens from the user’s own account.

5. Next Steps

We will proceed with removing authz dependencies as planned. If developers need similar functionality, they can rely on direct asset transfers to contracts, or explore custom modules. We will monitor community feedback and revisit the authz precompile approach if demand warrants it in the future. #

@cloudgray cloudgray linked a pull request Apr 9, 2025 that will close this issue
9 tasks
@cloudgray
Copy link

cloudgray commented Apr 10, 2025

Modification of Staking Precompile Integration Test

TL;DR

  • For direct calls to the staking precompile:
    • Simply remove test cases that involve approve.
  • For calls via a Solidity contract (StakingCaller):
    • Both the StakingCaller contract and its associated test cases must be updated together.
  • Updated test scenario:
    • The current flow:
      1. approve the StakingCaller contract
      2. delegate via the contract
    • The updated flow:
      1. transfer governance tokens to the StakingCaller contract
      2. delegate via the contract.

Test scenario change

Currently, the integration tests for the staking precompile are largely divided into two parts:

  1. Directly calling the staking precompile
  2. Calling staking precompile via solidity contract(StakingCaller)

Modifying the direct call approach is relatively simple, since it only requires removing the test cases that involve approve.

The part that requires more extensive changes is the one using the StakingCaller contract. This is because both the implementation and the call scenarios of the StakingCaller need to be updated together.

Calls made through the StakingCaller can be further divided into two scenarios.

Scenario 1

Previous:

  • The EoA first sends an approve transaction to the StakingCaller.
  • Then, in a separate transaction, it calls the delegate method.
  • The delegator address in this case is the EoA itself.

Updated:

  • The EoA sends a transfer of native tokens to the StakingCaller.
  • It then calls a delegate method on the StakingCaller.
  • The StakingCaller becomes the actual delegator, while it internally keeps track of the EoA and delegated amount.

Scenario 2

Previous:

  • The EoA calls a single method on the StakingCaller which internally executes both approve and delegate in one transaction.
  • Again, the delegator is the EoA.

Updated:

  • The EoA calls a payable method on the StakingCaller that combines the native token transfer and the delegate operation in a single call.
  • As before, the StakingCaller becomes the delegator and manages the mapping of EoA addresses to delegated amounts.

StakingCaller contract implementation change

Remove dependency on approve and use native token transfer instead:

  • The existing logic that called approve and delegate within a single transaction will be updated to use a payable function.
  • This function will receive native coins via transfer and use them to perform the delegate operation.

Track EOA balances internally:

  • A mapping from EOA addresses to delegated amounts should be maintained within the contract to track ownership and withdrawals.

@vladjdk
Copy link
Member Author

vladjdk commented Apr 10, 2025

I think payable is completely acceptable in this scenario. As far as I can tell, the current staking implementation falls under the assumption that the staking token should be treated as an ERC20.

I think it makes total sense to have the staking implementation work with the native gas token instead.

One question on the StakingCaller contract. It seems to only exist test approvals and staking on behalf of the user. I also think that reimplementing the functionality of the contract was a good call and still reflects the name pretty well.

Lgtm :)

@cloudgray
Copy link

@vladjdk
Thank you for your feedback and question.

If I may add, the approve mentioned in the message above refers specifically to the staking precompile’s approve method using x/authz - to be removed, not the ERC20 approve.
Test case does not treat the staking token as an ERC20.

Also, in both the current and updated test cases, staking is tested using the native gas token.
(In the test suite, the gas token denom is also used as x/staking.params.bond_denom - ref: configureAndInitChain)

“One question on the StakingCaller contract. It seems to only exist to test approvals and staking on behalf of the user.”
→ Yes, that’s basically correct. Additionally, if you look at the comments on the functions, you’ll see that another purpose of the StakingCaller contract is to test for any potential bugs when staking precompile methods are called in the same transaction with the other state change.
It ensures that the EVM state and Cosmos native state are properly updated or reverted as expected. [ref1] [ref2]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants