Skip to content

Commit

Permalink
imp(erc20): minor alignments between ERC20 extension and contracts (e…
Browse files Browse the repository at this point in the history
…vmos#2067)

* adjust approval behavior when approving 0

* add error vars for erc20 extension

* add helper to align errors with ERC20 errors

* add changelog entry

* add missing license

* check error

* move error to errors file

* further error refactors
  • Loading branch information
MalteHerrmann authored Nov 23, 2023
1 parent 35fe239 commit ac21bae
Show file tree
Hide file tree
Showing 9 changed files with 142 additions and 33 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
- (osmosis-outpost) [#2063](https://github.com/evmos/evmos/pull/2063) Check that receiver address is bech32 with "evmos" as human readable part.
- (cmn-precompile) [#2064](https://github.com/evmos/evmos/pull/2064) Handle all `fallback` and `receive` function cases
- (erc20) [#2066](https://github.com/evmos/evmos/pull/2066) Adjust ERC20 EVM extension allowance behavior to align with standard ERC20 smart contracts.
- (erc20) [#2067](https://github.com/evmos/evmos/pull/2067) Further alignments between ERC20 smart contracts and EVM extension.

### Bug Fixes

Expand Down
29 changes: 14 additions & 15 deletions precompiles/erc20/approve.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ import (
// operation succeeded and emits the Approval event on success.
//
// The Approve method handles 4 cases:
// 1. no authorization, amount 0 or negative -> return error
// 1. no authorization, amount negative -> return error
// 2. no authorization, amount positive -> create a new authorization
// 3. authorization exists, amount 0 or negative -> delete authorization
// 4. authorization exists, amount positive -> update authorization
// 5. no authorizaiton, amount 0 -> no-op but still emit Approval event
func (p Precompile) Approve(
ctx sdk.Context,
contract *vm.Contract,
Expand All @@ -49,11 +50,9 @@ func (p Precompile) Approve(
authorization, expiration, _ := auth.CheckAuthzExists(ctx, p.AuthzKeeper, grantee, granter, SendMsgURL) //#nosec:G703 -- we are handling the error case (authorization == nil) in the switch statement below

switch {
case authorization == nil && amount != nil && amount.Sign() <= 0:
case authorization == nil && amount != nil && amount.Sign() < 0:
// case 1: no authorization, amount 0 or negative -> error
// TODO: (@fedekunze) check if this is correct by comparing behavior with
// regular ERC20
err = errors.New("cannot approve non-positive values")
err = ErrNegativeAmount
case authorization == nil && amount != nil && amount.Sign() > 0:
// case 2: no authorization, amount positive -> create a new authorization
err = p.createAuthorization(ctx, grantee, granter, amount)
Expand Down Expand Up @@ -115,7 +114,7 @@ func (p Precompile) IncreaseAllowance(
// case 1: addedValue 0 or negative -> error
// TODO: (@fedekunze) check if this is correct by comparing behavior with
// regular ERC20
err = errors.New("cannot increase allowance with non-positive values")
err = ErrIncreaseNonPositiveValue
case authorization == nil && addedValue != nil && addedValue.Sign() > 0:
// case 2: no authorization, amount positive -> create a new authorization
amount = addedValue
Expand Down Expand Up @@ -174,10 +173,10 @@ func (p Precompile) DecreaseAllowance(
switch {
case subtractedValue != nil && subtractedValue.Sign() <= 0:
// case 1. subtractedValue 0 or negative -> return error
err = errors.New("cannot decrease allowance with non-positive values")
err = ErrDecreaseNonPositiveValue
case err != nil:
// case 2. no authorization -> return error
err = sdkerrors.Wrapf(err, "allowance does not exist")
err = sdkerrors.Wrap(err, fmt.Sprintf(ErrNoAllowanceForToken, p.tokenPair.Denom))
case subtractedValue != nil && subtractedValue.Cmp(allowance) < 0:
// case 3. subtractedValue positive and subtractedValue less than allowance -> update authorization
amount, err = p.decreaseAllowance(ctx, grantee, granter, subtractedValue, authorization, expiration)
Expand All @@ -187,10 +186,10 @@ func (p Precompile) DecreaseAllowance(
amount = common.Big0
case subtractedValue != nil && allowance.Sign() == 0:
// case 5. subtractedValue positive but no allowance for given denomination -> return error
err = fmt.Errorf("allowance for token %s does not exist", p.tokenPair.Denom)
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("subtracted value cannot be greater than existing allowance: %s > %s", subtractedValue, allowance)
err = fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance)
}

if err != nil {
Expand All @@ -207,7 +206,7 @@ func (p Precompile) DecreaseAllowance(

func (p Precompile) createAuthorization(ctx sdk.Context, grantee, granter common.Address, amount *big.Int) error {
if amount.BitLen() > sdkmath.MaxBitLen {
return fmt.Errorf("amount %s causes integer overflow", amount)
return fmt.Errorf(ErrIntegerOverflow, amount)
}

coins := sdk.Coins{{Denom: p.tokenPair.Denom, Amount: sdk.NewIntFromBigInt(amount)}}
Expand Down Expand Up @@ -242,13 +241,13 @@ func (p Precompile) removeSpendLimitOrDeleteAuthorization(ctx sdk.Context, grant

found, denomCoins := sendAuthz.SpendLimit.Find(p.tokenPair.Denom)
if !found {
return fmt.Errorf("allowance for token %s does not exist", p.tokenPair.Denom)
return fmt.Errorf(ErrNoAllowanceForToken, p.tokenPair.Denom)
}

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("subtracted value cannot be greater than existing allowance for denom %s: %s > %s",
return fmt.Errorf(ErrSubtractMoreThanAllowance,
p.tokenPair.Denom, denomCoins, sendAuthz.SpendLimit,
)
}
Expand Down Expand Up @@ -301,13 +300,13 @@ func (p Precompile) decreaseAllowance(

found, allowance := sendAuthz.SpendLimit.Find(p.tokenPair.Denom)
if !found {
return nil, errors.New("allowance for token does not exist")
return nil, fmt.Errorf(ErrNoAllowanceForToken, p.tokenPair.Denom)
}

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("subtracted value cannot be greater than existing allowance: %s > %s", subtractedValue, allowance.Amount)
return nil, fmt.Errorf(ErrSubtractMoreThanAllowance, p.tokenPair.Denom, subtractedValue, allowance.Amount)
}

if err := p.updateAuthorization(ctx, grantee, granter, amount, sendAuthz, expiration); err != nil {
Expand Down
15 changes: 8 additions & 7 deletions precompiles/erc20/approve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/vm"
"github.com/evmos/evmos/v15/precompiles/authorization"
cmn "github.com/evmos/evmos/v15/precompiles/common"
"github.com/evmos/evmos/v15/precompiles/erc20"
"github.com/evmos/evmos/v15/precompiles/testutil"
)

Expand Down Expand Up @@ -63,7 +65,7 @@ func (s *PrecompileTestSuite) TestApprove() {
s.keyring.GetAddr(1), big.NewInt(-1),
}
},
errContains: "cannot approve non-positive values",
errContains: erc20.ErrNegativeAmount.Error(),
},
{
name: "fail - approve uint256 overflow",
Expand Down Expand Up @@ -91,7 +93,7 @@ func (s *PrecompileTestSuite) TestApprove() {
s.keyring.GetAddr(1), common.Big0,
}
},
errContains: fmt.Sprintf("allowance for token %s does not exist", s.tokenDenom),
errContains: fmt.Sprintf(erc20.ErrNoAllowanceForToken, s.tokenDenom),
postCheck: func() {
// NOTE: Here we check that the authorization was not adjusted
s.requireSendAuthz(
Expand Down Expand Up @@ -314,7 +316,7 @@ func (s *PrecompileTestSuite) TestIncreaseAllowance() {
s.keyring.GetAddr(1), big.NewInt(-1),
}
},
errContains: "cannot increase allowance with non-positive values",
errContains: erc20.ErrIncreaseNonPositiveValue.Error(),
},
{
name: "pass - increase allowance without existing authorization",
Expand Down Expand Up @@ -374,7 +376,7 @@ func (s *PrecompileTestSuite) TestIncreaseAllowance() {
s.keyring.GetAddr(1), big.NewInt(amount),
}
},
errContains: "integer overflow when increasing allowance",
errContains: cmn.ErrIntegerOverflow,
postCheck: func() {
s.requireSendAuthz(
s.keyring.GetAccAddr(1),
Expand Down Expand Up @@ -512,7 +514,7 @@ func (s *PrecompileTestSuite) TestDecreaseAllowance() {
s.keyring.GetAddr(1), big.NewInt(-1),
}
},
errContains: "cannot decrease allowance with non-positive values",
errContains: erc20.ErrDecreaseNonPositiveValue.Error(),
},
{
name: "fail - decrease allowance without existing authorization",
Expand Down Expand Up @@ -677,7 +679,7 @@ func (s *PrecompileTestSuite) TestDecreaseAllowance() {
s.keyring.GetAddr(1), big.NewInt(decreaseAmount),
}
},
errContains: fmt.Sprintf("allowance for token %s does not exist", s.tokenDenom),
errContains: fmt.Sprintf(erc20.ErrNoAllowanceForToken, s.tokenDenom),
postCheck: func() {
// NOTE: Here we check that the authorization for the other denom was not deleted
s.requireSendAuthz(
Expand All @@ -701,7 +703,6 @@ func (s *PrecompileTestSuite) TestDecreaseAllowance() {
s.keyring.GetAddr(1), big.NewInt(decreaseAmount),
}
},
// TODO: have more verbose error message here like "authorization for different denomination found"?
errContains: "subtracted value cannot be greater than existing allowance",
postCheck: func() {
// NOTE: Here we check that the authorization was not adjusted
Expand Down
82 changes: 82 additions & 0 deletions precompiles/erc20/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// Copyright Tharsis Labs Ltd.(Evmos)
// SPDX-License-Identifier:ENCL-1.0(https://github.com/evmos/evmos/blob/main/LICENSE)

package erc20

import (
"errors"
"strings"

"github.com/cosmos/cosmos-sdk/x/authz"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/crypto"
evmtypes "github.com/evmos/evmos/v15/x/evm/types"
)

// Errors that have formatted information are defined here as a string.
const (
ErrIntegerOverflow = "amount %s causes integer overflow"
ErrNoAllowanceForToken = "allowance for token %s does not exist"
ErrSubtractMoreThanAllowance = "subtracted value cannot be greater than existing allowance for denom %s: %s > %s"
)

var (
// errorSignature are the prefix bytes for the hex-encoded reason string. See UnpackRevert in ABI implementation in Geth.
errorSignature = crypto.Keccak256([]byte("Error(string)"))

// Precompile errors
ErrDecreaseNonPositiveValue = errors.New("cannot decrease allowance with non-positive values")
ErrDenomTraceNotFound = errors.New("denom trace not found")
ErrIncreaseNonPositiveValue = errors.New("cannot increase allowance with non-positive values")
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.
ErrTransferAmountExceedsBalance = errors.New("ERC20: transfer amount exceeds balance")
)

// BuildExecRevertedErr returns a mocked error that should align with the
// behavior of the original ERC20 Solidity implementation.
//
// FIXME: This is not yet producing the correct reason bytes. Will fix on a follow up PR.
func BuildExecRevertedErr(reason string) (error, error) {
// This is reverse-engineering the ABI encoding of the revert reason.
typ, err := abi.NewType("string", "", nil)
if err != nil {
return nil, err
}

packedReason, err := (abi.Arguments{{Type: typ}}).Pack(reason)
if err != nil {
return nil, errors.New("failed to pack revert reason")
}

var reasonBytes []byte
reasonBytes = append(reasonBytes, errorSignature...)
reasonBytes = append(reasonBytes, packedReason...)

return evmtypes.NewExecErrorWithReason(reasonBytes), nil
}

// 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 {
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
default:
return err
}
}
25 changes: 25 additions & 0 deletions precompiles/erc20/errors_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package erc20_test

import (
"github.com/evmos/evmos/v15/precompiles/erc20"
evmtypes "github.com/evmos/evmos/v15/x/evm/types"
)

// TODO: This is not yet producing the correct reason bytes so we skip this test for now,
// until that's correctly implemented.
func (s *PrecompileTestSuite) TestBuildExecRevertedError() {
s.T().Skip("skipping until correctly implemented")

reason := "ERC20: transfer amount exceeds balance"
revErr, err := erc20.BuildExecRevertedErr(reason)
s.Require().NoError(err, "should not error when building revert error")

revertErr, ok := revErr.(*evmtypes.RevertError)
s.Require().True(ok, "error should be a revert error")

// Here we expect the correct revert reason that's returned by an ERC20 Solidity contract.
s.Require().Equal(
"0x08c379a00000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002645524332303a207472616e7366657220616d6f756e7420657863656564732062616c616e63650000000000000000000000000000000000000000000000000000",
revertErr.ErrorData(),
"error data should be the revert reason")
}
6 changes: 3 additions & 3 deletions precompiles/erc20/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
package erc20

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

errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"
authzkeeper "github.com/cosmos/cosmos-sdk/x/authz/keeper"
Expand Down Expand Up @@ -214,7 +214,7 @@ func GetDenomTrace(
denom string,
) (transfertypes.DenomTrace, error) {
if !strings.HasPrefix(denom, "ibc/") {
return transfertypes.DenomTrace{}, fmt.Errorf("denom is not an IBC voucher: %s", denom)
return transfertypes.DenomTrace{}, errorsmod.Wrapf(ErrNoIBCVoucherDenom, denom)
}

hash, err := transfertypes.ParseHexHash(denom[4:])
Expand All @@ -224,7 +224,7 @@ func GetDenomTrace(

denomTrace, found := transferKeeper.GetDenomTrace(ctx, hash)
if !found {
return transfertypes.DenomTrace{}, errors.New("denom trace not found")
return transfertypes.DenomTrace{}, ErrDenomTraceNotFound
}

return denomTrace, nil
Expand Down
12 changes: 6 additions & 6 deletions precompiles/erc20/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func (s *PrecompileTestSuite) TestNameSymbol() {
{
name: "fail - empty denom",
denom: "",
errContains: "denom is not an IBC voucher",
errContains: erc20.ErrNoIBCVoucherDenom.Error(),
},
{
name: "fail - invalid denom trace",
Expand All @@ -129,7 +129,7 @@ func (s *PrecompileTestSuite) TestNameSymbol() {
{
name: "fail - denom not found",
denom: types.DenomTrace{Path: "channel-0", BaseDenom: "notfound"}.IBCDenom(),
errContains: "denom trace not found",
errContains: erc20.ErrDenomTraceNotFound.Error(),
},
{
name: "fail - invalid denom (too short < 3 chars)",
Expand All @@ -142,7 +142,7 @@ func (s *PrecompileTestSuite) TestNameSymbol() {
{
name: "fail - denom without metadata and not an IBC voucher",
denom: "noIBCvoucher",
errContains: "denom is not an IBC voucher",
errContains: erc20.ErrNoIBCVoucherDenom.Error(),
},
{
name: "pass - valid ibc denom without metadata and neither atto nor micro prefix",
Expand Down Expand Up @@ -236,7 +236,7 @@ func (s *PrecompileTestSuite) TestDecimals() {
{
name: "fail - empty denom",
denom: "",
errContains: "denom is not an IBC voucher",
errContains: erc20.ErrNoIBCVoucherDenom.Error(),
},
{
name: "fail - invalid denom trace",
Expand All @@ -246,12 +246,12 @@ func (s *PrecompileTestSuite) TestDecimals() {
{
name: "fail - denom not found",
denom: types.DenomTrace{Path: "channel-0", BaseDenom: "notfound"}.IBCDenom(),
errContains: "denom trace not found",
errContains: erc20.ErrDenomTraceNotFound.Error(),
},
{
name: "fail - denom without metadata and not an IBC voucher",
denom: "noIBCvoucher",
errContains: "denom is not an IBC voucher",
errContains: erc20.ErrNoIBCVoucherDenom.Error(),
},
{
name: "fail - valid ibc denom without metadata and neither atto nor micro prefix",
Expand Down
1 change: 1 addition & 0 deletions precompiles/erc20/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func (p Precompile) transfer(
}

if err != nil {
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
Loading

0 comments on commit ac21bae

Please sign in to comment.