8000 ENG 377 Delayed malicious effects by danburck · Pull Request #192 · evmos/evmos · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ENG 377 Delayed malicious effects #192

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jan 6, 2022
Merged

ENG 377 Delayed malicious effects #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9f4f466
bug(erc20): Add monitorApprovalEvent to conversion methods
danburck Jan 4, 2022
9979303
check for approve event on for native ERC20'
danburck Jan 4, 2022
bb81283
Update Spec
danburck Jan 4, 2022
c26e21a
Update CHANGELOG
danburck Jan 4, 2022
7d17f76
WIP test contract
danburck Jan 5, 2022
6a9659d
WIP test contract
danburck Jan 5, 2022
ddf03f8
bug(erc20): Add malicious contract for testing
danburck Jan 5, 2022
4dba1d0
bug(erc20): Update tests with malicious contract
danburck Jan 5, 2022
626d5a1
Merge branch 'main' into ENG-377-Delayed-malicious-effects
danburck Jan 5, 2022
90d0bb2
Address comments
danburck Jan 5, 2022
0726270
Update x/erc20/keeper/msg_server.go
fedekunze Jan 5, 2022
9c4477e
bug(erc20): Fix monnitorApprovalEvent method
danburck Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
- Sanitize the ERC20 token name when creating coin metadata on ER `RegisterERC20Proposal`.
- Fix coin metadata validation error when registering an ERC20 with 0 denom units.
- (erc20) [\#191](https://github.com/tharsis/evmos/pull/191) Add direct balance protection (IF-ETHERMINT-06).

- (erc20) [\#192](https://github.com/tharsis/evmos/pull/192) Add delayed malicious effect protection (IF-ETHERMINT-06).

## [v0.4.2] - 2021-12-11

Expand Down
6 changes: 3 additions & 3 deletions x/erc20/keeper/evm_hooks_test.go
8000
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (suite *KeeperTestSuite) TestEvmHooksRegisterERC20() {
suite.Require().NoError(err)

// Mint 10 tokens to suite.address (owner)
_ = suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10))
_ = suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10), false)
suite.Commit()

// Burn the 10 tokens of suite.address (owner)
Expand All @@ -48,7 +48,7 @@ func (suite *KeeperTestSuite) TestEvmHooksRegisterERC20() {
"unregistered pair",
func(contractAddr common.Address) {
// Mint 10 tokens to suite.address (owner)
_ = suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10))
_ = suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10), false)
suite.Commit()

// Burn the 10 tokens of suite.address (owner)
Expand All @@ -66,7 +66,7 @@ func (suite *KeeperTestSuite) TestEvmHooksRegisterERC20() {
suite.Require().NoError(err)

// Mint 10 tokens to suite.address (owner)
msg := suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10))
msg := suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(10), false)
hash := msg.AsTransaction().Hash()
logs := suite.app.EvmKeeper.GetTxLogsTransient(hash)
suite.Require().NotEmpty(logs)
Expand Down
53 changes: 51 additions & 2 deletions x/erc20/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,49 @@ func (suite *KeeperTestSuite) DeployContract(name string, symbol string) common.
return crypto.CreateAddress(suite.address, nonce)
}

func (suite *KeeperTestSuite) DeployContractMaliciousDelayed(name string, symbol string) common.Address {
ctx := sdk.WrapSDKContext(suite.ctx)
chainID := suite.app.EvmKeeper.ChainID()

ctorArgs, err := contracts.ERC20MaliciousDelayedContract.ABI.Pack("", big.NewInt(1000000000000000000))
suite.Require().NoError(err)

data := append(contracts.ERC20MaliciousDelayedContract.Bin, ctorArgs...)
args, err := json.Marshal(&evm.TransactionArgs{
From: &suite.address,
Data: (*hexutil.Bytes)(&data),
})
suite.Require().NoError(err)

res, err := suite.queryClientEvm.EstimateGas(ctx, &evm.EthCallRequest{
Args: args,
GasCap: uint64(config.DefaultGasCap),
})
suite.Require().NoError(err)

nonce := suite.app.EvmKeeper.GetNonce(suite.address)

erc20DeployTx := evm.NewTxContract(
chainID,
nonce,
nil, // amount
res.Gas, // gasLimit
nil, // gasPrice
suite.app.FeeMarketKeeper.GetBaseFee(suite.ctx),
big.NewInt(1),
data, // input
&ethtypes.AccessList{}, // accesses
)

erc20DeployTx.From = suite.address.Hex()
err = erc20DeployTx.Sign(ethtypes.LatestSignerForChainID(chainID), suite.signer)
suite.Require().NoError(err)
rsp, err := suite.app.EvmKeeper.EthereumTx(ctx, erc20DeployTx)
suite.Require().NoError(err)
suite.Require().Empty(rsp.VmError)
return crypto.CreateAddress(suite.address, nonce)
}

func (suite *KeeperTestSuite) Commit() {
_ = suite.app.Commit()
header := suite.ctx.BlockHeader()
Expand All @@ -226,8 +269,14 @@ func (suite *KeeperTestSuite) Commit() {
suite.queryClientEvm = evm.NewQueryClient(queryHelper)
}

func (suite *KeeperTestSuite) MintERC20Token(contractAddr, from, to common.Address, amount *big.Int) *evm.MsgEthereumTx {
transferData, err := contracts.ERC20BurnableAndMintableContract.ABI.Pack("mint", to, amount)
func (suite *KeeperTestSuite) MintERC20Token(contractAddr, from, to common.Address, amount *big.Int, isMaliciousDelayed bool) *evm.MsgEthereumTx {
var transferData []byte
var err error
if isMaliciousDelayed {
transferData, err = contracts.ERC20MaliciousDelayedContract.ABI.Pack("mint", to, amount)
} else {
transferData, err = contracts.ERC20BurnableAndMintableContract.ABI.Pack("mint", to, amount)
}
suite.Require().NoError(err)
return suite.sendTx(contractAddr, from, transferData)
}
Expand Down
38 changes: 38 additions & 0 deletions x/erc20/keeper/msg_server.go
View file
Open in desktop
9E88
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/ethereum/go-ethereum/accounts/abi"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
evmtypes "github.com/tharsis/ethermint/x/evm/types"

"github.com/tharsis/evmos/x/erc20/types"
Expand Down Expand Up @@ -216,6 +217,11 @@ func (k Keeper) convertCoinNativeERC20(
)
}

// Check for unexpected `appove` event in logs
if err := k.monitorApprovalEvent(res); err != nil {
return nil, err
}

ctx.EventManager().EmitEvents(
sdk.Events{
sdk.NewEvent(
Expand Down Expand Up @@ -306,6 +312,7 @@ func (k Keeper) convertERC20NativeCoin(
// - Send minted coins to the receiver
// - Check if coin balance increased by amount
// - Check if token balance decreased by amount
// - Check for unexpected `appove` event in logs
func (k Keeper) convertERC20NativeToken(
ctx sdk.Context,
pair types.TokenPair,
Expand Down Expand Up @@ -373,6 +380,11 @@ func (k Keeper) convertERC20NativeToken(
)
}

// Check for unexpected `appove` event in logs
if err := k.monitorApprovalEvent(res); err != nil {
return nil, err
}

ctx.EventManager().EmitEvents(
sdk.Events{
sdk.NewEvent(
Expand All @@ -389,6 +401,7 @@ func (k Keeper) convertERC20NativeToken(
return &types.MsgConvertERC20Response{}, nil
}

// balanceOf queries an account's balance for a given ERC20 contract
func (k Keeper) balanceOf(
ctx sdk.Context,
abi abi.ABI,
Expand All @@ -411,3 +424,28 @@ func (k Keeper) balanceOf(

return balance
}

// monitorApprovalEvent returns an error if the given transactions logs include
// an unexpected `approve` event
func (k Keeper) monitorApprovalEvent(res *evmtypes.MsgEthereumTxResponse) error {
if res == nil {
return nil
}
// TODO: fetch from response
hash := common.BytesToHash([]byte(res.Hash))
logs := k.evmKeeper.GetTxLogsTransient(hash)
if len(logs) == 0 {
return nil
}
logApprovalSig := []byte("Approval(address,address,uint256)")
logApprovalSigHash := crypto.Keccak256Hash(logApprovalSig)

for _, log := range logs {
if log.Topics[0].Hex() == logApprovalSigHash.Hex() {
return sdkerrors.Wrapf(
types.ErrUnexpectedEvent, "unexpected approval event",
)
}
}
return nil
}
88 changes: 64 additions & 24 deletions x/erc20/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,19 @@ func (suite *KeeperTestSuite) TestConvertECR20NativeCoin() {
}

func (suite *KeeperTestSuite) TestConvertECR20NativeERC20() {
var contractAddr common.Address

testCases := []struct {
name string
mint int64
burn int64
malleate func(common.Address)
expPass bool
name string
mint int64
burn int64
malleate func(common.Address)
isMaliciousDelayed bool
expPass bool
}{
{"ok - sufficient funds", 100, 10, func(common.Address) {}, true},
{"ok - equal funds", 10, 10, func(common.Address) {}, true},
{"ok - equal funds", 10, 10, func(common.Address) {}, true},
{"ok - sufficient funds", 100, 10, func(common.Address) {}, false, true},
{"ok - equal funds", 10, 10, func(common.Address) {}, false, true},
{"ok - equal funds", 10, 10, func(common.Address) {}, false, true},
{
"ok - suicided contract",
10,
Expand All @@ -190,9 +193,10 @@ func (suite *KeeperTestSuite) TestConvertECR20NativeERC20() {
suite.app.EvmKeeper.SetCode(erc20, []byte{})
suite.Commit()
},
false,
true,
},
{"fail - insufficient funds - callEVM", 0, 10, func(common.Address) {}, false},
{"fail - insufficient funds - callEVM", 0, 10, func(common.Address) {}, false, false},
{
"fail - minting disabled",
100,
Expand All @@ -203,13 +207,24 @@ func (suite *KeeperTestSuite) TestConvertECR20NativeERC20() {
suite.app.Erc20Keeper.SetParams(suite.ctx, params)
},
false,
false,
},
{
"fail - delayed malicious contract",
10,
10,
func(common.Address) {
contractAddr = suite.setupRegisterERC20PairMaliciousDelayed()
},
true,
false,
},
}
for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
suite.mintFeeCollector = true
suite.SetupTest()
contractAddr := suite.setupRegisterERC20Pair()
contractAddr = suite.setupRegisterERC20Pair()
suite.Require().NotNil(contractAddr)

tc.malleate(contractAddr)
Expand All @@ -223,7 +238,8 @@ func (suite *KeeperTestSuite) TestConvertECR20NativeERC20() {
contractAddr,
suite.address,
)
suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(tc.mint))

suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(tc.mint), tc.isMaliciousDelayed)
suite.Commit()
ctx := sdk.WrapSDKContext(suite.ctx)

Expand Down Expand Up @@ -255,29 +271,48 @@ func (suite *KeeperTestSuite) TestConvertECR20NativeERC20() {
}

func (suite *KeeperTestSuite) TestConvertCoinNativeERC20() {
var contractAddr common.Address

testCases := []struct {
name string
mint int64
burn int64
reconvert int64
expPass bool
name string
mint int64
burn int64
reconvert int64
malleate func(common.Address)
isMaliciousDelayed bool
expPass bool
}{
{"ok - sufficient funds", 100, 10, 5, true},
{"ok - equal funds", 10, 10, 10, true},
{"fail - insufficient funds", 10, 1, 5, false},
{"ok - sufficient funds", 100, 10, 5, func(common.Address) {}, false, true},
{"ok - equal funds", 10, 10, 10, func(common.Address) {}, false, true},
{"fail - insufficient funds", 10, 1, 5, func(common.Address) {}, false, false},
{
"fail - delayed malicious contract",
100,
10,
5,
func(common.Address) {
contractAddr = suite.setupRegisterERC20PairMaliciousDelayed()
},
true,
false,
},
}
for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.name), func() {
suite.mintFeeCollector = true
suite.SetupTest()
contractAddr := suite.setupRegisterERC20Pair()
contractAddr = suite.setupRegisterERC20Pair()
suite.Require().NotNil(contractAddr)

tc.malleate(contractAddr)
suite.Commit()

// Precondition: Convert ERC20 to Coins
coinName := types.CreateDenom(contractAddr.String())
sender := sdk.AccAddress(suite.address.Bytes())
suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(tc.mint))
suite.MintERC20Token(contractAddr, suite.address, suite.address, big.NewInt(tc.mint), tc.isMaliciousDelayed)
suite.Commit()

msgConvertERC20 := types.NewMsgConvertERC20(
sdk.NewInt(tc.burn),
sender,
Expand All @@ -287,12 +322,17 @@ func (suite *KeeperTestSuite) TestConvertCoinNativeERC20() {

ctx := sdk.WrapSDKContext(suite.ctx)
_, err := suite.app.Erc20Keeper.ConvertERC20(ctx, msgConvertERC20)
suite.Require().NoError(err)
suite.Commit()
balance := suite.BalanceOf(contractAddr, suite.address)
cosmosBalance := suite.app.BankKeeper.GetBalance(suite.ctx, sender, coinName)
suite.Require().Equal(cosmosBalance.Amount, sdk.NewInt(tc.burn))
suite.Require().Equal(balance.(*big.Int).Int64(), big.NewInt(tc.mint-tc.burn).Int64())

if tc.isMaliciousDelayed {
suite.Require().Error(err)
} else {
suite.Require().NoError(err)
suite.Require().Equal(cosmosBalance.Amount, sdk.NewInt(tc.burn))
suite.Require().Equal(balance.(*big.Int).Int64(), big.NewInt(tc.mint-tc.burn).Int64())
}

// Convert Coins back to ERC20s
ctx = sdk.WrapSDKContext(suite.ctx)
Expand Down
8 changes: 8 additions & 0 deletions x/erc20/keeper/proposals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ func (suite *KeeperTestSuite) setupRegisterERC20Pair() common.Address {
suite.Require().NoError(err)
return contractAddr
}
func (suite *KeeperTestSuite) setupRegisterERC20PairMaliciousDelayed() common.Address {
suite.SetupTest()
contractAddr := suite.DeployContractMaliciousDelayed(erc20Name, erc20Symbol)
suite.Commit()
_, err := suite.app.Erc20Keeper.RegisterERC20(suite.ctx, contractAddr)
suite.Require().NoError(err)
return contractAddr
}

func (suite *KeeperTestSuite) setupRegisterCoin() (banktypes.Metadata, *types.TokenPair) {
suite.SetupTest()
Expand Down
2 changes: 2 additions & 0 deletions x/erc20/spec/03_state_transitions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ Conversion of a registered `TokenPair` can be done via:
4. Check if
- Coin balance increased by amount
- Token balance decreased by amount
5. Fail if unexpected `appove` event found in logs

#### 2.2 Coin to ERC20

Expand All @@ -113,3 +114,4 @@ Conversion of a registered `TokenPair` can be done via:
2. Unlock escrowed ERC20 from the module address by sending it to the recipient
3. Burn escrowed Cosmos coins
4. Check if token balance increased by amount
5. Fail if unexpected `appove` event found in logs
Loading
0