Skip to content

Commit

Permalink
imp(erc20): align allowance adjustment errors with erc20 contracts (e…
Browse files Browse the repository at this point in the history
…vmos#2075)

* align allowance adjustment errors with erc20

* add changelog entry
  • Loading branch information
MalteHerrmann authored Nov 24, 2023
1 parent 71c2f0b commit 84ca6b3
Show file tree
Hide file tree
Showing 6 changed files with 27 additions and 22 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 5 additions & 5 deletions precompiles/erc20/approve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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() {
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
7 changes: 4 additions & 3 deletions precompiles/erc20/approve_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package erc20_test

import (
"errors"
"fmt"
"math/big"

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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(
Expand Down
17 changes: 10 additions & 7 deletions precompiles/erc20/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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")
)

Expand All @@ -63,20 +62,24 @@ 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
case strings.Contains(err.Error(), "requested amount is more than spend limit"):
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") ||
Expand Down
12 changes: 6 additions & 6 deletions precompiles/erc20/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:]
Expand All @@ -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:])
Expand All @@ -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
Expand All @@ -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,
))
Expand All @@ -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,
))
Expand Down
2 changes: 1 addition & 1 deletion precompiles/erc20/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down

0 comments on commit 84ca6b3

Please sign in to comment.