From 84ca6b35874b208c49086b62d11e34135299b9eb Mon Sep 17 00:00:00 2001 From: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Date: Fri, 24 Nov 2023 17:44:49 +0100 Subject: [PATCH] imp(erc20): align allowance adjustment errors with erc20 contracts (#2075) * align allowance adjustment errors with erc20 * add changelog entry --- CHANGELOG.md | 1 + precompiles/erc20/approve.go | 10 +++++----- precompiles/erc20/approve_test.go | 7 ++++--- precompiles/erc20/errors.go | 17 ++++++++++------- precompiles/erc20/query.go | 12 ++++++------ precompiles/erc20/tx.go | 2 +- 6 files changed, 27 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca0c6f1c28..c71821dcba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,6 +108,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ - (stride-outpost) [#2069](https://github.com/evmos/evmos/pull/2069) Refactor `osmosis` and `stride` testing. - (erc20) [#2073](https://github.com/evmos/evmos/pull/2073) Align metadata query errors with ERC20 contracts. - (werc20) [#2074](https://github.com/evmos/evmos/pull/2074) Add `werc20` EVM Extension acceptance tests with `WEVMOS` contract. +- (erc20) [#2075](https://github.com/evmos/evmos/pull/2075) Align allowance adjustment errors with ERC20 contracts. ### Bug Fixes diff --git a/precompiles/erc20/approve.go b/precompiles/erc20/approve.go index b4dda38740..1358a993f3 100644 --- a/precompiles/erc20/approve.go +++ b/precompiles/erc20/approve.go @@ -189,7 +189,7 @@ func (p Precompile) DecreaseAllowance( err = fmt.Errorf(ErrNoAllowanceForToken, p.tokenPair.Denom) case subtractedValue != nil && subtractedValue.Cmp(allowance) > 0: // case 6. subtractedValue positive and subtractedValue higher than allowance -> return error - err = fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance) + err = ConvertErrToERC20Error(fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance)) } if err != nil { @@ -247,9 +247,9 @@ func (p Precompile) removeSpendLimitOrDeleteAuthorization(ctx sdk.Context, grant newSpendLimit, hasNeg := sendAuthz.SpendLimit.SafeSub(denomCoins) // NOTE: safety check only, this should never happen since we only subtract what was found in the slice. if hasNeg { - return fmt.Errorf(ErrSubtractMoreThanAllowance, + return ConvertErrToERC20Error(fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, denomCoins, sendAuthz.SpendLimit, - ) + )) } if newSpendLimit.IsZero() { @@ -276,7 +276,7 @@ func (p Precompile) increaseAllowance( sdkAddedValue := sdk.NewIntFromBigInt(addedValue) amount, overflow := cmn.SafeAdd(allowance, sdkAddedValue) if overflow { - return nil, errors.New(cmn.ErrIntegerOverflow) + return nil, ConvertErrToERC20Error(errors.New(cmn.ErrIntegerOverflow)) } if err := p.updateAuthorization(ctx, grantee, granter, amount, sendAuthz, expiration); err != nil { @@ -306,7 +306,7 @@ func (p Precompile) decreaseAllowance( amount = new(big.Int).Sub(allowance.Amount.BigInt(), subtractedValue) // NOTE: Safety check only since this is checked in the DecreaseAllowance method already. if amount.Sign() < 0 { - return nil, fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance.Amount) + return nil, ConvertErrToERC20Error(fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance.Amount)) } if err := p.updateAuthorization(ctx, grantee, granter, amount, sendAuthz, expiration); err != nil { diff --git a/precompiles/erc20/approve_test.go b/precompiles/erc20/approve_test.go index 90db3142c7..78f8a904ea 100644 --- a/precompiles/erc20/approve_test.go +++ b/precompiles/erc20/approve_test.go @@ -1,6 +1,7 @@ package erc20_test import ( + "errors" "fmt" "math/big" @@ -376,7 +377,7 @@ func (s *PrecompileTestSuite) TestIncreaseAllowance() { s.keyring.GetAddr(1), big.NewInt(amount), } }, - errContains: cmn.ErrIntegerOverflow, + errContains: erc20.ConvertErrToERC20Error(errors.New(cmn.ErrIntegerOverflow)).Error(), postCheck: func() { s.requireSendAuthz( s.keyring.GetAccAddr(1), @@ -664,7 +665,7 @@ func (s *PrecompileTestSuite) TestDecreaseAllowance() { s.keyring.GetAddr(1), big.NewInt(amount + 1), } }, - errContains: "subtracted value cannot be greater than existing allowance", + errContains: erc20.ConvertErrToERC20Error(errors.New("subtracted value cannot be greater than existing allowance")).Error(), }, { name: "fail - decrease allowance with existing authorization in different denomination", @@ -703,7 +704,7 @@ func (s *PrecompileTestSuite) TestDecreaseAllowance() { s.keyring.GetAddr(1), big.NewInt(decreaseAmount), } }, - errContains: "subtracted value cannot be greater than existing allowance", + errContains: erc20.ConvertErrToERC20Error(errors.New("subtracted value cannot be greater than existing allowance")).Error(), postCheck: func() { // NOTE: Here we check that the authorization was not adjusted s.requireSendAuthz( diff --git a/precompiles/erc20/errors.go b/precompiles/erc20/errors.go index e1a3a378d3..f437ecb590 100644 --- a/precompiles/erc20/errors.go +++ b/precompiles/erc20/errors.go @@ -11,6 +11,7 @@ import ( "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/crypto" + cmn "github.com/evmos/evmos/v15/precompiles/common" evmtypes "github.com/evmos/evmos/v15/x/evm/types" ) @@ -32,11 +33,9 @@ var ( ErrNegativeAmount = errors.New("cannot approve negative values") ErrNoIBCVoucherDenom = errors.New("denom is not an IBC voucher") - // ErrInsufficientAllowance is returned by ERC20 smart contracts in case - // no authorization is found or the granted amount is not sufficient. - ErrInsufficientAllowance = errors.New("ERC20: insufficient allowance") - // ErrTransferAmountExceedsBalance is returned by ERC20 smart contracts in - // case a transfer is attempted, that sends more than the sender's balance. + // ERC20 errors + ErrDecreasedAllowanceBelowZero = errors.New("ERC20: decreased allowance below zero") + ErrInsufficientAllowance = errors.New("ERC20: insufficient allowance") ErrTransferAmountExceedsBalance = errors.New("ERC20: transfer amount exceeds balance") ) @@ -63,13 +62,13 @@ func BuildExecRevertedErr(reason string) (error, error) { return evmtypes.NewExecErrorWithReason(reasonBytes), nil } -// convertErrToERC20Error is a helper function which maps errors raised by the Cosmos SDK stack +// ConvertErrToERC20Error is a helper function which maps errors raised by the Cosmos SDK stack // to the corresponding errors which are raised by an ERC20 contract. // // TODO: Create the full RevertError types instead of just the standard error type. // // TODO: Return ERC-6093 compliant errors. -func convertErrToERC20Error(err error) error { +func ConvertErrToERC20Error(err error) error { switch { case strings.Contains(err.Error(), "spendable balance"): return ErrTransferAmountExceedsBalance @@ -77,6 +76,10 @@ func convertErrToERC20Error(err error) error { return ErrInsufficientAllowance case strings.Contains(err.Error(), authz.ErrNoAuthorizationFound.Error()): return ErrInsufficientAllowance + case strings.Contains(err.Error(), "subtracted value cannot be greater than existing allowance"): + return ErrDecreasedAllowanceBelowZero + case strings.Contains(err.Error(), cmn.ErrIntegerOverflow): + return vm.ErrExecutionReverted case errors.Is(err, ErrNoIBCVoucherDenom) || errors.Is(err, ErrDenomTraceNotFound) || strings.Contains(err.Error(), "invalid base denomination") || diff --git a/precompiles/erc20/query.go b/precompiles/erc20/query.go index 6eaf1eb29a..ffede65b5e 100644 --- a/precompiles/erc20/query.go +++ b/precompiles/erc20/query.go @@ -59,7 +59,7 @@ func (p Precompile) Name( baseDenom, err := p.getBaseDenomFromIBCVoucher(ctx, p.tokenPair.Denom) if err != nil { - return nil, convertErrToERC20Error(err) + return nil, ConvertErrToERC20Error(err) } name := strings.ToUpper(string(baseDenom[1])) + baseDenom[2:] @@ -83,7 +83,7 @@ func (p Precompile) Symbol( baseDenom, err := p.getBaseDenomFromIBCVoucher(ctx, p.tokenPair.Denom) if err != nil { - return nil, convertErrToERC20Error(err) + return nil, ConvertErrToERC20Error(err) } symbol := strings.ToUpper(baseDenom[1:]) @@ -104,7 +104,7 @@ func (p Precompile) Decimals( if !found { denomTrace, err := GetDenomTrace(p.transferKeeper, ctx, p.tokenPair.Denom) if err != nil { - return nil, convertErrToERC20Error(err) + return nil, ConvertErrToERC20Error(err) } // we assume the decimal from the first character of the denomination @@ -114,7 +114,7 @@ func (p Precompile) Decimals( case "a": // atto (a) -> 18 decimals return method.Outputs.Pack(uint8(18)) } - return nil, convertErrToERC20Error(fmt.Errorf( + return nil, ConvertErrToERC20Error(fmt.Errorf( "invalid base denomination; should be either micro ('u[...]') or atto ('a[...]'); got: %q", denomTrace.BaseDenom, )) @@ -133,14 +133,14 @@ func (p Precompile) Decimals( } if !displayFound { - return nil, convertErrToERC20Error(fmt.Errorf( + return nil, ConvertErrToERC20Error(fmt.Errorf( "display denomination not found for denom: %q", p.tokenPair.Denom, )) } if decimals > math.MaxUint8 { - return nil, convertErrToERC20Error(fmt.Errorf( + return nil, ConvertErrToERC20Error(fmt.Errorf( "uint8 overflow: invalid decimals: %d", decimals, )) diff --git a/precompiles/erc20/tx.go b/precompiles/erc20/tx.go index 0f80024ce9..0b6e12dd47 100644 --- a/precompiles/erc20/tx.go +++ b/precompiles/erc20/tx.go @@ -90,7 +90,7 @@ func (p Precompile) transfer( } if err != nil { - err = convertErrToERC20Error(err) + err = ConvertErrToERC20Error(err) // This should return an error to avoid the contract from being executed and an event being emitted return nil, err }