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

fix(erc20): adjust ERC20 allowance behavior #2066

Merged
merged 7 commits into from Nov 21, 2023

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Nov 21, 2023

Description

This PR adjusts some characteristics of the ERC20 EVM extension to align with the standard ERC20 smart contracts. The changes were motivated by the results of testing in #2044:

  • Instead of returning errors for allowances that are not found, we return zero.
  • Instead of using the contract.CallerAddress for creating and adjusting approvals, we pass the tx.Origin in order to create the correct approvals between the users. Currently, the approvals are instead created between a calling smart contract and the grantee, instead of the desired granter to grantee.

@MalteHerrmann MalteHerrmann requested a review from a team as a code owner November 21, 2023 13:49
@MalteHerrmann MalteHerrmann requested review from Vvaradinov and 0xstepit and removed request for a team November 21, 2023 13:49
@MalteHerrmann
Copy link
Contributor Author

Addressing failing tests now

@Vvaradinov
Copy link
Contributor

I'm not sure this change is in alignment with the default ERC20 behaviour. It seems the approve function does take the msg.sender or contract.CallerAddress in our case? What if the contract is using it's own balance against the ERC20 precompile (but it's triggered by an origin) it would look for a grant between the 2 contracts (ERC20 <-> custom contract)

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #2066 (4050afb) into main (b0f0098) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2066      +/-   ##
==========================================
- Coverage   70.16%   70.16%   -0.01%     
==========================================
  Files         340      340              
  Lines       25453    25455       +2     
==========================================
+ Hits        17860    17861       +1     
- Misses       6675     6676       +1     
  Partials      918      918              
Files Coverage Δ
precompiles/erc20/query.go 95.86% <50.00%> (-0.78%) ⬇️

@MalteHerrmann
Copy link
Contributor Author

@Vvaradinov agreed, that was the point I was raising in our sync yesterday. The decision we will have to make is between us wanting to allow smart contracts to be able to be used for updating allowances for a granter/grantee combination which would align with our other precompiles or if we want to have the usage of msg.Sender as in the ERC20 original?

@MalteHerrmann
Copy link
Contributor Author

After further discussion and research with @Vvaradinov the changes regarding using the transaction origin will be reverted, since that behavior is just like that on the actual OpenZeppelin ERC20 implementations.

We must note however, that the resulting behavior on the ERC20 precompile is different from our other precompiles, that include approvals. Here, when calling a smart contract, that in turn calls the Approve method on an ERC20 contract, the approval is not made between the EOA initiating the transaction and the grantee, but instead between SC and the grantee. This is interesting behavior, since one does not need to be the contract owner to set up approvals for the smart contract.

@MalteHerrmann MalteHerrmann changed the title fix(erc20): adjust ERC20 approval behavior fix(erc20): adjust ERC20 allowance behavior Nov 21, 2023
@fedekunze fedekunze merged commit 9c01f06 into main Nov 21, 2023
33 of 34 checks passed
@fedekunze fedekunze deleted the malte/adjust-erc20-allowance-query branch November 21, 2023 17:01
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 this pull request may close these issues.

None yet

3 participants