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(str): Add some changes from the internal STRv2 audit #2443

Merged

Conversation

MalteHerrmann
Copy link
Contributor

@MalteHerrmann MalteHerrmann commented Mar 29, 2024

Description

This PR adjusts some things found in the internal STR v2 audit:

  • The handling of the maxUint256 approvals for the ERC-20 transfers was not correctly implemented. The bank module's SendAuthorization does not have the same notion of an unlimited approval (check here) as e.g. the StakeAuthorization does (see here and here). Hence, the handling for this ERC-20 characteristic has to happen on the level of the EVM extension.
  • Passing the empty address (common.Address{}) did not return errors before, even though it is the case for the transfer, transferFrom and approve methods on a standard ERC-20 Solidity contract by OpenZeppelin.
  • During the OnRecvPacket in the ERC-20 IBC middleware, we were wrongfully checking if the sender address was a module address, when rather it should be checked for the receiver.
  • Fix some test logic for the IBC callbacks.
  • Use IsPositive() rather than IsZero() || IsNegative().

precompiles/erc20/tx_test.go Outdated Show resolved Hide resolved
precompiles/erc20/integration_test.go Outdated Show resolved Hide resolved
@MalteHerrmann MalteHerrmann marked this pull request as ready for review April 12, 2024 09:02
@MalteHerrmann MalteHerrmann requested a review from a team as a code owner April 12, 2024 09:02
@MalteHerrmann MalteHerrmann requested review from Vvaradinov, 0xstepit, ramacarlucho, facs95 and GAtom22 and removed request for a team April 12, 2024 09:02
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

LGTM!! Great work @MalteHerrmann!!

Copy link
Contributor

@Vvaradinov Vvaradinov left a comment

Choose a reason for hiding this comment

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

LGTM!

@MalteHerrmann
Copy link
Contributor Author

The failing tests are the same failling ones as on the base branch after merging #2476, which is okay as those are going to be removed in a follow-up PR. 👍

@MalteHerrmann MalteHerrmann merged commit 1717667 into Vvaradinov/feat-strv2-feature-branch Apr 16, 2024
34 of 36 checks passed
@MalteHerrmann MalteHerrmann deleted the malte/strv2-audit-adjustments branch April 16, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants