fix(str): Apply changes from internal audit #2476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ramacarlucho
requested review from
facs95 and
0xstepit
and removed request for
a team
ramacarlucho
changed the title
GAtom22
reviewed
Apr 8, 2024
There was a problem hiding this comment.
Great work @ramacarlucho!!
Please, address linter issues
Also, tests are failing
Vvaradinov
reviewed
Apr 9, 2024
fedekunze
reviewed
Apr 9, 2024
x/erc20/keeper/msg_server.go
Outdated
| ) | ||
|
|
||
| return &types.MsgConvertERC20Response{}, nil | ||
| return nil, types.ErrUndefinedOwner | ||
| } | ||
|
|
||
| // convertERC20NativeToken handles the erc20 conversion for a native erc20 token |
There was a problem hiding this comment.
comment needs to be updated
GAtom22
approved these changes
Apr 9, 2024
MalteHerrmann
approved these changes
Apr 10, 2024
x/erc20/keeper/msg_server.go
Outdated
Comment on lines
44
to
48
| if pair.IsNativeERC20() { | ||
| return k.convertERC20IntoCoinsForNativeToken(ctx, pair, msg, receiver, sender) // case 2.1 | ||
| } | ||
|
|
||
| // Check expected receiver balance after transfer | ||
| balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) | ||
| expCoin := balanceCoin.Add(coins[0]) | ||
| if ok := balanceCoinAfter.IsEqual(expCoin); !ok { | ||
| return nil, errorsmod.Wrapf( | ||
| types.ErrBalanceInvariance, | ||
| "invalid coin balance - expected: %v, actual: %v", | ||
| expCoin, balanceCoinAfter, | ||
| ) | ||
| } | ||
|
|
||
| // Check expected Sender balance after transfer | ||
| tokens := coins[0].Amount.BigInt() | ||
| balanceTokenAfter := k.BalanceOf(ctx, erc20, contract, sender) | ||
| if balanceTokenAfter == nil { | ||
| return nil, errorsmod.Wrap(types.ErrEVMCall, "failed to retrieve balance") | ||
| } | ||
|
|
||
| expToken := big.NewInt(0).Sub(balanceToken, tokens) | ||
| if r := balanceTokenAfter.Cmp(expToken); r != 0 { | ||
| return nil, errorsmod.Wrapf( | ||
| types.ErrBalanceInvariance, | ||
| "invalid token balance - expected: %v, actual: %v", | ||
| expToken, balanceTokenAfter, | ||
| ) | ||
| } | ||
|
|
||
| defer func() { | ||
| telemetry.IncrCounterWithLabels( | ||
| []string{"tx", "msg", "convert", "erc20", "total"}, | ||
| 1, | ||
| []metrics.Label{ | ||
| telemetry.NewLabel("denom", pair.Denom), | ||
| }, | ||
| ) | ||
|
|
||
| if msg.Amount.IsInt64() { | ||
| telemetry.IncrCounterWithLabels( | ||
| []string{"tx", "msg", "convert", "erc20", "amount", "total"}, | ||
| float32(msg.Amount.Int64()), | ||
| []metrics.Label{ | ||
| telemetry.NewLabel("denom", pair.Denom), | ||
| }, | ||
| ) | ||
| } | ||
| }() | ||
|
|
||
| ctx.EventManager().EmitEvents( | ||
| sdk.Events{ | ||
| sdk.NewEvent( | ||
| types.EventTypeConvertERC20, | ||
| sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender), | ||
| sdk.NewAttribute(types.AttributeKeyReceiver, msg.Receiver), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, msg.Amount.String()), | ||
| sdk.NewAttribute(types.AttributeKeyCosmosCoin, pair.Denom), | ||
| sdk.NewAttribute(types.AttributeKeyERC20Token, msg.ContractAddress), | ||
| ), | ||
| }, | ||
| ) | ||
|
|
||
| return &types.MsgConvertERC20Response{}, nil | ||
| return nil, types.ErrUndefinedOwner |
There was a problem hiding this comment.
the error raised in case it's not a native ERC-20 is incorrect for the native coins, because it claims that the owner is undefined.
We should rather check if it's not an ERC-20 and then return a corresponding error:
Suggested change
| if pair.IsNativeERC20() { | |
| return k.convertERC20IntoCoinsForNativeToken(ctx, pair, msg, receiver, sender) // case 2.1 | |
| } | |
| // Check expected receiver balance after transfer | |
| balanceCoinAfter := k.bankKeeper.GetBalance(ctx, receiver, pair.Denom) | |
| expCoin := balanceCoin.Add(coins[0]) | |
| if ok := balanceCoinAfter.IsEqual(expCoin); !ok { | |
| return nil, errorsmod.Wrapf( | |
| types.ErrBalanceInvariance, | |
| "invalid coin balance - expected: %v, actual: %v", | |
| expCoin, balanceCoinAfter, | |
| ) | |
| } | |
| // Check expected Sender balance after transfer | |
| tokens := coins[0].Amount.BigInt() | |
| balanceTokenAfter := k.BalanceOf(ctx, erc20, contract, sender) | |
| if balanceTokenAfter == nil { | |
| return nil, errorsmod.Wrap(types.ErrEVMCall, "failed to retrieve balance") | |
| } | |
| expToken := big.NewInt(0).Sub(balanceToken, tokens) | |
| if r := balanceTokenAfter.Cmp(expToken); r != 0 { | |
| return nil, errorsmod.Wrapf( | |
| types.ErrBalanceInvariance, | |
| "invalid token balance - expected: %v, actual: %v", | |
| expToken, balanceTokenAfter, | |
| ) | |
| } | |
| defer func() { | |
| telemetry.IncrCounterWithLabels( | |
| []string{"tx", "msg", "convert", "erc20", "total"}, | |
| 1, | |
| []metrics.Label{ | |
| telemetry.NewLabel("denom", pair.Denom), | |
| }, | |
| ) | |
| if msg.Amount.IsInt64() { | |
| telemetry.IncrCounterWithLabels( | |
| []string{"tx", "msg", "convert", "erc20", "amount", "total"}, | |
| float32(msg.Amount.Int64()), | |
| []metrics.Label{ | |
| telemetry.NewLabel("denom", pair.Denom), | |
| }, | |
| ) | |
| } | |
| }() | |
| ctx.EventManager().EmitEvents( | |
| sdk.Events{ | |
| sdk.NewEvent( | |
| types.EventTypeConvertERC20, | |
| sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender), | |
| sdk.NewAttribute(types.AttributeKeyReceiver, msg.Receiver), | |
| sdk.NewAttribute(sdk.AttributeKeyAmount, msg.Amount.String()), | |
| sdk.NewAttribute(types.AttributeKeyCosmosCoin, pair.Denom), | |
| sdk.NewAttribute(types.AttributeKeyERC20Token, msg.ContractAddress), | |
| ), | |
| }, | |
| ) | |
| return &types.MsgConvertERC20Response{}, nil | |
| return nil, types.ErrUndefinedOwner | |
| if !pair.IsNativeERC20() { | |
| return nil, errors.New("converting erc-20s to coins is only supported for native erc-20 tokens") | |
| } | |
| return k.convertERC20IntoCoinsForNativeToken(ctx, pair, msg, receiver, sender) |
We also need to delete or adjust the ErrUndefinedOwner in the types if it's not used somewhere else
There was a problem hiding this comment.
ErrUndefinedOwner is used when its an empty token pair.
x/erc20/keeper/msg_server.go
Outdated
Comment on lines
66
to
77
| // Remove token pair if contract is suicided | ||
| acc := k.evmKeeper.GetAccountWithoutBalance(ctx, pair.GetERC20Contract()) | ||
| if acc == nil || !acc.IsContract() { | ||
| k.DeleteTokenPair(ctx, pair) | ||
| k.Logger(ctx).Debug( | ||
| "deleting selfdestructed token pair from state", | ||
| "contract", pair.Erc20Address, | ||
| ) | ||
| // NOTE: return nil error to persist the changes from the deletion | ||
| return nil, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
hmm IMO this should remain on the higher level of abstraction in ConvertERC20 - was there a specific reason to move it into this method?
There was a problem hiding this comment.
moved it outside the function, but inside the pair.IsNativeERC20 check to garantee safety
x/erc20/keeper/msg_server.go
Outdated
| @@ -210,6 +105,8 @@ func (k Keeper) convertERC20NativeToken( | |||
| } | |||
|
|
|||
| // Check expected escrow balance after transfer execution | |||
| // NOTE: coin fields already validated | |||
There was a problem hiding this comment.
nit: would be good to mention where it was validated
Suggested change
| // NOTE: coin fields already validated | |
| // NOTE: coin fields already validated in the ValidateBasic() of the message |
| @@ -34,23 +35,24 @@ func TestPrecompileTestSuite(t *testing.T) { | |||
|
|
|||
| func (s *PrecompileTestSuite) SetupTest() { | |||
There was a problem hiding this comment.
it's not possible for me to comment in that line but in line 20 of this file there is: var _ *PrecompileTestSuite which we should remove I think
Comment on lines
368
to
+370
|
|
||
| err = evmosutils.FundAccountWithBaseDenom(s.unitNetwork.GetContext(), s.unitNetwork.App.BankKeeper, sender, 1000000000000000000) | ||
| s.Require().NoError(err, "failed to fund account") |
There was a problem hiding this comment.
why is this necessary? We should rather set up the test suite with enough funds or adjust the test case to make use of the existing balance I think - also not sure where this amount comes from, why exactly 1000000000000000000?
precompiles/bank/setup_test.go
Outdated
| @@ -32,7 +32,7 @@ type PrecompileTestSuite struct { | |||
| network *network.UnitTestNetwork | |||
| factory factory.TxFactory | |||
| grpcHandler grpc.Handler | |||
| keyring testkeyring.Keyring | |||
There was a problem hiding this comment.
i would leave this as it was, when renaming the import we are shadowing the import name with the keyring variable below
Suggested change
| keyring testkeyring.Keyring | |
| keyring testkeyring.Keyring |
ramacarlucho
merged commit 6d5d101
into
Vvaradinov/feat-strv2-feature-branch
31 of 33 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes: EVM-63
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.md