diff --git a/.github/labeler.yml b/.github/labeler.yml index aebf63e968..293796511c 100644 --- a/.github/labeler.yml +++ b/.github/labeler.yml @@ -4,3 +4,6 @@ breaking:proto: breaking:cli: - "x/*/client/cli/*.go" - "cmd/**/*.go" + +ci: + - ".github/**" diff --git a/.github/workflows/sast-linters.yml b/.github/workflows/sast-linters.yml index 8b86ea3c6c..79efac1562 100644 --- a/.github/workflows/sast-linters.yml +++ b/.github/workflows/sast-linters.yml @@ -33,10 +33,33 @@ jobs: # uses: ./.github/actions/install-dependencies - name: Run Gosec Security Scanner - run: | - export PATH=$PATH:$(go env GOPATH)/bin - go install github.com/securego/gosec/v2/cmd/gosec@latest - gosec ./... + uses: securego/gosec@master + with: + args: ./... + + gosec-cosmos: + runs-on: ubuntu-latest + env: + GO111MODULE: on + steps: + - name: Checkout Source + uses: actions/checkout@v2 + with: + fetch-depth: 0 + + - name: Set up Go + uses: actions/setup-go@v3 + with: + go-version: '1.20' + + # - name: Install Pipeline Dependencies + # uses: ./.github/actions/install-dependencies + + - name: Run Cosmos Gosec Security Scanner + uses: cosmos/gosec@master + with: + args: './... -include=G701,G703,G704' # Disabled G702 as it doesn't seem to be relevant 2023-09-14 + git-guardian: runs-on: ubuntu-latest @@ -68,18 +91,18 @@ jobs: with: fetch-depth: 0 - - name: Install Pipeline Dependencies - uses: ./.github/actions/install-dependencies + # - name: Install Pipeline Dependencies + # uses: ./.github/actions/install-dependencies - name: Set up Go uses: actions/setup-go@v3 with: - go-version: '1.19' + go-version: '1.20' - name: Run golangci-lint uses: golangci/golangci-lint-action@v3 with: - version: v1.50 + version: v1.54 skip-cache: true args: --timeout=15m @@ -137,8 +160,11 @@ jobs: Be very careful about using `#nosec` in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way. + Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 + Broad `#nosec` annotations should be avoided, as they can hide other vulnerabilities. **The CI will block you from merging this PR until you remove `#nosec` annotations that do not target specific rules**. + Pay extra attention to the way `#nosec` is being used in the files listed above. - + - name: Add Label uses: actions/github-script@v6 if: env.nosec_detected == 1 @@ -150,3 +176,11 @@ jobs: repo: context.repo.repo, labels: ["nosec"] }) + + - name: Check for '#nosec' without a specific rule + run: | + DIFF=$(git diff ${{ github.event.pull_request.base.sha }}) + echo "$DIFF" | grep -P '#nosec(?!(\sG\d{3}))(?![^\s\t])([\s\t]*|$)' && echo "nosec without specified rule found!" && exit 1 || exit 0 + + + \ No newline at end of file diff --git a/x/crosschain/keeper/evm_hooks.go b/x/crosschain/keeper/evm_hooks.go index 260df6dc3a..5ef345e7c3 100644 --- a/x/crosschain/keeper/evm_hooks.go +++ b/x/crosschain/keeper/evm_hooks.go @@ -4,10 +4,12 @@ import ( "bytes" "encoding/hex" "fmt" + "math/big" "strings" errorsmod "cosmossdk.io/errors" "cosmossdk.io/math" + "github.com/btcsuite/btcutil" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/ethereum/go-ethereum/accounts/abi" "github.com/ethereum/go-ethereum/accounts/abi/bind" @@ -256,10 +258,28 @@ func (k Keeper) ParseZRC20WithdrawalEvent(ctx sdk.Context, log ethtypes.Log) (*z return nil, err } - _, found := k.fungibleKeeper.GetForeignCoins(ctx, event.Raw.Address.Hex()) + coin, found := k.fungibleKeeper.GetForeignCoins(ctx, event.Raw.Address.Hex()) if !found { return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: cannot find foreign coin with contract address %s", event.Raw.Address.Hex()) } + chainID := coin.ForeignChainId + if common.IsBitcoinChain(chainID) { + if event.Value.Cmp(big.NewInt(0)) <= 0 { + return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid amount %s", event.Value.String()) + } + btcChainParams, err := common.GetBTCChainParams(chainID) + if err != nil { + return nil, err + } + addr, err := btcutil.DecodeAddress(string(event.To), btcChainParams) + if err != nil { + return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s: %s", event.To, err) + } + _, ok := addr.(*btcutil.AddressWitnessPubKeyHash) + if !ok { + return nil, fmt.Errorf("ParseZRC20WithdrawalEvent: invalid address %s (not P2WPKH address)", event.To) + } + } return event, nil } diff --git a/x/fungible/keeper/evm.go b/x/fungible/keeper/evm.go index 731b57cad6..9911c81adf 100644 --- a/x/fungible/keeper/evm.go +++ b/x/fungible/keeper/evm.go @@ -3,6 +3,7 @@ package keeper import ( "encoding/hex" "encoding/json" + "errors" "fmt" "math/big" "strconv" @@ -226,6 +227,43 @@ func (k Keeper) DepositZRC20AndCallContract(ctx sdk.Context, "depositAndCall", context, zrc20Addr, amount, targetContract, message) } +// QueryProtocolFlatFee returns the protocol flat fee associated with a given zrc20 +func (k Keeper) QueryProtocolFlatFee(ctx sdk.Context, contract common.Address) (*big.Int, error) { + zrc20ABI, err := zrc20.ZRC20MetaData.GetAbi() + if err != nil { + return nil, err + } + res, err := k.CallEVM( + ctx, + *zrc20ABI, + types.ModuleAddressEVM, + contract, + BigIntZero, + nil, + false, + false, + "PROTOCOL_FLAT_FEE", + ) + if err != nil { + return nil, err + } + + unpacked, err := zrc20ABI.Unpack("PROTOCOL_FLAT_FEE", res.Ret) + if err != nil { + return nil, err + } + if len(unpacked) == 0 { + return nil, fmt.Errorf("expect 1 returned values, got %d", len(unpacked)) + } + + protocolGasFee, ok := unpacked[0].(*big.Int) + if !ok { + return nil, errors.New("can't read returned value as big.Int") + } + + return protocolGasFee, nil +} + // QueryZRC20Data returns the data of a deployed ZRC20 contract func (k Keeper) QueryZRC20Data( ctx sdk.Context, diff --git a/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee.go b/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee.go index dc6d18864a..56e75381e6 100644 --- a/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee.go +++ b/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee.go @@ -2,7 +2,8 @@ package keeper import ( "context" - "math/big" + + cosmoserrors "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -14,55 +15,50 @@ import ( func (k Keeper) UpdateZRC20WithdrawFee(goCtx context.Context, msg *types.MsgUpdateZRC20WithdrawFee) (*types.MsgUpdateZRC20WithdrawFeeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) + + // check signer permission if msg.Creator != k.observerKeeper.GetParams(ctx).GetAdminPolicyAccount(zetaObserverTypes.Policy_Type_deploy_fungible_coin) { - return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "Deploy can only be executed by the correct policy account") + return nil, cosmoserrors.Wrap(sdkerrors.ErrUnauthorized, "deploy can only be executed by the correct policy account") } + + // check the zrc20 exists zrc20Addr := ethcommon.HexToAddress(msg.Zrc20Address) if zrc20Addr == (ethcommon.Address{}) { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid zrc20 contract address (%s)", msg.Zrc20Address) - } - - // update contracts - zrc20ABI, err := zrc20.ZRC20MetaData.GetAbi() - if err != nil { - return nil, sdkerrors.Wrapf(types.ErrABIGet, "failed to get zrc20 abi") - } - - foreignCoins := k.GetAllForeignCoins(ctx) - found := false - var coin types.ForeignCoins - for _, fcoin := range foreignCoins { - coinZRC20Addr := ethcommon.HexToAddress(fcoin.Zrc20ContractAddress) - if coinZRC20Addr == (ethcommon.Address{}) { - k.Logger(ctx).Error("invalid zrc20 contract address", "address", fcoin.Zrc20ContractAddress) - continue - } - if coinZRC20Addr == zrc20Addr { - coin = fcoin - found = true - break - } + return nil, cosmoserrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid zrc20 contract address (%s)", msg.Zrc20Address) } - + coin, found := k.GetForeignCoins(ctx, msg.Zrc20Address) if !found { - return nil, sdkerrors.Wrapf(types.ErrInvalidAddress, "no foreign coin match requested zrc20 address (%s)", msg.Zrc20Address) + return nil, cosmoserrors.Wrapf(types.ErrForeignCoinNotFound, "no foreign coin match requested zrc20 address (%s)", msg.Zrc20Address) } - res, err := k.CallEVM(ctx, *zrc20ABI, types.ModuleAddressEVM, zrc20Addr, BigIntZero, nil, false, false, "PROTOCOL_FLAT_FEE") + // get the previous fee + oldWithdrawFee, err := k.QueryProtocolFlatFee(ctx, zrc20Addr) if err != nil { - return nil, sdkerrors.Wrapf(types.ErrContractCall, "failed to call zrc20 contract PROTOCOL_FLAT_FEE method (%s)", err.Error()) - } - unpacked, err := zrc20ABI.Unpack("PROTOCOL_FLAT_FEE", res.Ret) - if err != nil || len(unpacked) == 0 { - return nil, sdkerrors.Wrapf(types.ErrContractCall, "failed to unpack zrc20 contract PROTOCOL_FLAT_FEE method (%s)", err.Error()) + return nil, cosmoserrors.Wrapf(types.ErrContractCall, "failed to query protocol flat fee (%s)", err.Error()) } - oldWithdrawFee, ok := unpacked[0].(*big.Int) - if !ok { - return nil, sdkerrors.Wrapf(types.ErrContractCall, "failed to interpret the returned unpacked zrc20 contract PROTOCOL_FLAT_FEE method; ret %x", res.Ret) + + zrc20ABI, err := zrc20.ZRC20MetaData.GetAbi() + if err != nil { + return nil, cosmoserrors.Wrapf(types.ErrABIGet, "failed to get zrc20 abi") } + // call the contract method to update the fee tmpCtx, commit := ctx.CacheContext() - _, err = k.CallEVM(tmpCtx, *zrc20ABI, types.ModuleAddressEVM, zrc20Addr, BigIntZero, nil, true, false, "updateProtocolFlatFee", msg.NewWithdrawFee.BigInt()) + _, err = k.CallEVM( + tmpCtx, + *zrc20ABI, + types.ModuleAddressEVM, + zrc20Addr, + BigIntZero, + nil, + true, + false, + "updateProtocolFlatFee", + msg.NewWithdrawFee.BigInt(), + ) + if err != nil { + return nil, cosmoserrors.Wrapf(types.ErrContractCall, "failed to call zrc20 contract updateProtocolFlatFee method (%s)", err.Error()) + } err = ctx.EventManager().EmitTypedEvent( &types.EventZRC20WithdrawFeeUpdated{ @@ -77,8 +73,9 @@ func (k Keeper) UpdateZRC20WithdrawFee(goCtx context.Context, msg *types.MsgUpda ) if err != nil { k.Logger(ctx).Error("failed to emit event", "error", err.Error()) - return nil, sdkerrors.Wrapf(types.ErrEmitEvent, "failed to emit event (%s)", err.Error()) + return nil, cosmoserrors.Wrapf(types.ErrEmitEvent, "failed to emit event (%s)", err.Error()) } commit() + return &types.MsgUpdateZRC20WithdrawFeeResponse{}, nil } diff --git a/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee_test.go b/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee_test.go new file mode 100644 index 0000000000..28a967b248 --- /dev/null +++ b/x/fungible/keeper/msg_server_update_zrc20_withdraw_fee_test.go @@ -0,0 +1,159 @@ +package keeper_test + +import ( + "errors" + "math/big" + "testing" + + "cosmossdk.io/math" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + evmtypes "github.com/evmos/ethermint/x/evm/types" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "github.com/zeta-chain/protocol-contracts/pkg/contracts/zevm/zrc20.sol" + keepertest "github.com/zeta-chain/zetacore/testutil/keeper" + "github.com/zeta-chain/zetacore/testutil/sample" + "github.com/zeta-chain/zetacore/x/fungible/types" +) + +func TestKeeper_UpdateZRC20WithdrawFee(t *testing.T) { + t.Run("can update the withdraw fee", func(t *testing.T) { + k, ctx, sdkk, zk := keepertest.FungibleKeeper(t) + chainID := getValidChainID(t) + k.GetAuthKeeper().GetModuleAccount(ctx, types.ModuleName) + + // set coin admin + admin := sample.AccAddress() + setAdminDeployFungibleCoin(ctx, zk, admin) + + // deploy the system contract and a ZRC20 contract + deploySystemContracts(t, ctx, k, sdkk.EvmKeeper) + zrc20Addr := setupGasCoin(t, ctx, k, sdkk.EvmKeeper, chainID, "alpha", "alpha") + + // initial protocol fee is zero + fee, err := k.QueryProtocolFlatFee(ctx, zrc20Addr) + require.NoError(t, err) + require.Zero(t, fee.Uint64()) + + // can update the fee + _, err = k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + admin, + zrc20Addr.String(), + math.NewUint(42), + )) + require.NoError(t, err) + + // can query the updated fee + fee, err = k.QueryProtocolFlatFee(ctx, zrc20Addr) + require.NoError(t, err) + require.Equal(t, uint64(42), fee.Uint64()) + }) + + t.Run("should fail if not authorized", func(t *testing.T) { + k, ctx, _, _ := keepertest.FungibleKeeper(t) + + _, err := k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + sample.AccAddress(), + sample.EthAddress().String(), + math.NewUint(42)), + ) + require.ErrorIs(t, err, sdkerrors.ErrUnauthorized) + }) + + t.Run("should fail if invalid zrc20 address", func(t *testing.T) { + k, ctx, _, zk := keepertest.FungibleKeeper(t) + admin := sample.AccAddress() + setAdminDeployFungibleCoin(ctx, zk, admin) + + _, err := k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + admin, + "invalid_address", + math.NewUint(42)), + ) + require.ErrorIs(t, err, sdkerrors.ErrInvalidAddress) + }) + + t.Run("should fail if can't retrieve the foreign coin", func(t *testing.T) { + k, ctx, _, zk := keepertest.FungibleKeeper(t) + admin := sample.AccAddress() + setAdminDeployFungibleCoin(ctx, zk, admin) + + _, err := k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + admin, + sample.EthAddress().String(), + math.NewUint(42)), + ) + require.ErrorIs(t, err, types.ErrForeignCoinNotFound) + }) + + t.Run("should fail if can't query old fee", func(t *testing.T) { + k, ctx, _, zk := keepertest.FungibleKeeper(t) + k.GetAuthKeeper().GetModuleAccount(ctx, types.ModuleName) + + // setup + admin := sample.AccAddress() + setAdminDeployFungibleCoin(ctx, zk, admin) + zrc20 := sample.EthAddress() + k.SetForeignCoins(ctx, sample.ForeignCoins(t, zrc20.String())) + + // the method shall fail since we only set the foreign coin manually in the store but didn't deploy the contract + _, err := k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + admin, + zrc20.String(), + math.NewUint(42)), + ) + require.ErrorIs(t, err, types.ErrContractCall) + }) + + t.Run("should fail if contract call for setting new fee fails", func(t *testing.T) { + k, ctx, _, zk := keepertest.FungibleKeeperWithMocks(t, keepertest.FungibleMockOptions{UseEVMMock: true}) + k.GetAuthKeeper().GetModuleAccount(ctx, types.ModuleName) + mockEVMKeeper := keepertest.GetFungibleEVMMock(t, k) + + // setup + admin := sample.AccAddress() + setAdminDeployFungibleCoin(ctx, zk, admin) + zrc20Addr := sample.EthAddress() + k.SetForeignCoins(ctx, sample.ForeignCoins(t, zrc20Addr.String())) + + // evm mocks + mockEVMKeeper.On("EstimateGas", mock.Anything, mock.Anything).Maybe().Return( + &evmtypes.EstimateGasResponse{Gas: 1000}, + nil, + ) + mockEVMKeeper.On("WithChainID", mock.Anything).Maybe().Return(ctx) + mockEVMKeeper.On("ChainID").Maybe().Return(big.NewInt(1)) + + // this is the query (commit == false) + zrc20ABI, err := zrc20.ZRC20MetaData.GetAbi() + require.NoError(t, err) + protocolFlatFee, err := zrc20ABI.Methods["PROTOCOL_FLAT_FEE"].Outputs.Pack(big.NewInt(42)) + require.NoError(t, err) + mockEVMKeeper.On( + "ApplyMessage", + mock.Anything, + mock.Anything, + mock.Anything, + false, + ).Return(&evmtypes.MsgEthereumTxResponse{Ret: protocolFlatFee}, nil) + + // this is the update call (commit == true) + mockEVMKeeper.On( + "ApplyMessage", + mock.Anything, + mock.Anything, + mock.Anything, + true, + ).Return(&evmtypes.MsgEthereumTxResponse{}, errors.New("transaction failed")) + + _, err = k.UpdateZRC20WithdrawFee(ctx, types.NewMsgUpdateZRC20WithdrawFee( + admin, + zrc20Addr.String(), + math.NewUint(42)), + ) + require.ErrorIs(t, err, types.ErrContractCall) + + mockEVMKeeper.AssertExpectations(t) + }) +} diff --git a/x/fungible/types/message_update_zrc20_withdraw_fee.go b/x/fungible/types/message_update_zrc20_withdraw_fee.go index ba1c61183f..1b1d6de1ec 100644 --- a/x/fungible/types/message_update_zrc20_withdraw_fee.go +++ b/x/fungible/types/message_update_zrc20_withdraw_fee.go @@ -45,7 +45,7 @@ func (msg *MsgUpdateZRC20WithdrawFee) ValidateBasic() error { return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err) } // check if the system contract address is valid - if ethcommon.HexToAddress(msg.Zrc20Address) == (ethcommon.Address{}) { + if !ethcommon.IsHexAddress(msg.Zrc20Address) { return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid system contract address (%s)", msg.Zrc20Address) } if msg.NewWithdrawFee.IsNil() {