fix(Audit): Audit issues (backport #190) (#199)

Co-authored-by: David Terpay <[email protected]>
Co-authored-by: David Terpay <[email protected]>

.golangci.yml

@@ -10,7 +10,6 @@ run:
 linters:
   disable-all: true
   enable:
-    - depguard
     - dogsled
     - exportloopref
     - goconst

abci/abci_test.go

@@ -52,7 +52,7 @@ type ABCITestSuite struct {
 
 	// account set up
 	accounts []testutils.Account
-	balances sdk.Coins
+	balance  sdk.Coin
 	random   *rand.Rand
 	nonces   map[string]uint64
 }
@@ -100,8 +100,8 @@ func (suite *ABCITestSuite) SetupTest() {
 	suite.builderDecorator = ante.NewBuilderDecorator(suite.builderKeeper, suite.encodingConfig.TxConfig.TxEncoder(), suite.mempool)
 
 	// Accounts set up
-	suite.accounts = testutils.RandomAccounts(suite.random, 1)
-	suite.balances = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(1000000000000000000)))
+	suite.accounts = testutils.RandomAccounts(suite.random, 10)
+	suite.balance = sdk.NewCoin("foo", sdk.NewInt(1000000000000000000))
 	suite.nonces = make(map[string]uint64)
 	for _, acc := range suite.accounts {
 		suite.nonces[acc.Address.String()] = 0
@@ -114,7 +114,7 @@ func (suite *ABCITestSuite) SetupTest() {
 
 func (suite *ABCITestSuite) anteHandler(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) {
 	signer := tx.GetMsgs()[0].GetSigners()[0]
-	suite.bankKeeper.EXPECT().GetAllBalances(ctx, signer).AnyTimes().Return(suite.balances)
+	suite.bankKeeper.EXPECT().GetBalance(ctx, signer, suite.balance.Denom).AnyTimes().Return(suite.balance)
 
 	next := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) {
 		return ctx, nil

proto/pob/builder/v1/tx.proto

@@ -38,11 +38,11 @@ message MsgAuctionBid {
   string bidder = 1 [ (cosmos_proto.scalar) = "cosmos.AddressString" ];
   // bid is the amount of coins that the bidder is bidding to participate in the
   // auction.
-  cosmos.base.v1beta1.Coin bid = 3
+  cosmos.base.v1beta1.Coin bid = 2
       [ (gogoproto.nullable) = false, (amino.dont_omitempty) = true ];
   // transactions are the bytes of the transactions that the bidder wants to
   // bundle together.
-  repeated bytes transactions = 4;
+  repeated bytes transactions = 3;
 }
 
 // MsgAuctionBidResponse defines the Msg/AuctionBid response type.

testutils/mocks.go

@@ -58,16 +58,16 @@ func (m *MockBankKeeper) EXPECT() *MockBankKeeperMockRecorder {
 	return m.recorder
 }
 
-func (m *MockBankKeeper) GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins {
+func (m *MockBankKeeper) GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "GetAllBalances", ctx, addr)
-	ret0 := ret[0].(sdk.Coins)
+	ret := m.ctrl.Call(m, "GetBalance", ctx, addr, denom)
+	ret0 := ret[0].(sdk.Coin)
 	return ret0
 }
 
-func (mr *MockBankKeeperMockRecorder) GetAllBalances(ctx, addr interface{}) *gomock.Call {
+func (mr *MockBankKeeperMockRecorder) GetBalance(ctx, addr, denom interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllBalances", reflect.TypeOf((*MockBankKeeper)(nil).GetAllBalances), ctx, addr)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetBalance", reflect.TypeOf((*MockBankKeeper)(nil).GetBalance), ctx, addr, denom)
 }
 
 func (m *MockBankKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error {

x/builder/ante/ante_test.go

@@ -34,6 +34,9 @@ type AnteTestSuite struct {
 	builderDecorator ante.BuilderDecorator
 	key              *storetypes.KVStoreKey
 	authorityAccount sdk.AccAddress
+
+	// Account set up
+	balance sdk.Coin
 }
 
 func TestAnteTestSuite(t *testing.T) {
@@ -69,23 +72,23 @@ func (suite *AnteTestSuite) SetupTest() {
 	suite.Require().NoError(err)
 }
 
-func (suite *AnteTestSuite) executeAnteHandler(tx sdk.Tx, balance sdk.Coins) (sdk.Context, error) {
+func (suite *AnteTestSuite) anteHandler(ctx sdk.Context, tx sdk.Tx, _ bool) (sdk.Context, error) {
 	signer := tx.GetMsgs()[0].GetSigners()[0]
-	suite.bankKeeper.EXPECT().GetAllBalances(suite.ctx, signer).AnyTimes().Return(balance)
+	suite.bankKeeper.EXPECT().GetBalance(ctx, signer, suite.balance.Denom).AnyTimes().Return(suite.balance)
 
-	next := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (sdk.Context, error) {
+	next := func(ctx sdk.Context, tx sdk.Tx, _ bool) (sdk.Context, error) {
 		return ctx, nil
 	}
 
-	return suite.builderDecorator.AnteHandle(suite.ctx, tx, false, next)
+	return suite.builderDecorator.AnteHandle(ctx, tx, false, next)
 }
 
 func (suite *AnteTestSuite) TestAnteHandler() {
 	var (
 		// Bid set up
 		bidder  = testutils.RandomAccounts(suite.random, 1)[0]
 		bid     = sdk.NewCoin("foo", sdk.NewInt(1000))
-		balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+		balance = sdk.NewCoin("foo", sdk.NewInt(10000))
 		signers = []testutils.Account{bidder}
 
 		// Top bidding auction tx set up
@@ -125,14 +128,14 @@ func (suite *AnteTestSuite) TestAnteHandler() {
 			"bidder has insufficient balance, invalid auction tx",
 			func() {
 				insertTopBid = false
-				balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10)))
+				balance = sdk.NewCoin("foo", sdk.NewInt(10))
 			},
 			false,
 		},
 		{
 			"bid is smaller than reserve fee, invalid auction tx",
 			func() {
-				balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+				balance = sdk.NewCoin("foo", sdk.NewInt(10000))
 				bid = sdk.NewCoin("foo", sdk.NewInt(101))
 				reserveFee = sdk.NewCoin("foo", sdk.NewInt(1000))
 			},
@@ -141,7 +144,7 @@ func (suite *AnteTestSuite) TestAnteHandler() {
 		{
 			"valid auction bid tx",
 			func() {
-				balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+				balance = sdk.NewCoin("foo", sdk.NewInt(10000))
 				bid = sdk.NewCoin("foo", sdk.NewInt(1000))
 				reserveFee = sdk.NewCoin("foo", sdk.NewInt(100))
 			},
@@ -158,7 +161,7 @@ func (suite *AnteTestSuite) TestAnteHandler() {
 			"auction tx is the top bidding tx",
 			func() {
 				timeout = 1000
-				balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+				balance = sdk.NewCoin("foo", sdk.NewInt(10000))
 				bid = sdk.NewCoin("foo", sdk.NewInt(1000))
 				reserveFee = sdk.NewCoin("foo", sdk.NewInt(100))
 
@@ -249,7 +252,8 @@ func (suite *AnteTestSuite) TestAnteHandler() {
 
 			// Execute the ante handler
 			suite.builderDecorator = ante.NewBuilderDecorator(suite.builderKeeper, suite.encodingConfig.TxConfig.TxEncoder(), mempool)
-			_, err = suite.executeAnteHandler(auctionTx, balance)
+			suite.balance = balance
+			_, err = suite.anteHandler(suite.ctx, auctionTx, false)
 			if tc.pass {
 				suite.Require().NoError(err)
 			} else {

x/builder/client/cli/query.go

@@ -34,7 +34,11 @@ func CmdQueryParams() *cobra.Command {
 		Short: "Query the current parameters of the builder module",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			clientCtx := client.GetClientContextFromCmd(cmd)
+			clientCtx, err := client.GetClientQueryContext(cmd)
+			if err != nil {
+				return err
+			}
+
 			queryClient := types.NewQueryClient(clientCtx)
 
 			request := &types.QueryParamsRequest{}

x/builder/client/cli/tx.go

@@ -42,7 +42,9 @@ where each transaction is a hex-encoded string of a signed transaction.
 		Args:    cobra.ExactArgs(3),
 		Example: "auction-bid cosmos1... 10000uatom 0xFF...,0xCC...,0xAA...",
 		RunE: func(cmd *cobra.Command, args []string) error {
-			cmd.Flags().Set(flags.FlagFrom, args[0])
+			if err := cmd.Flags().Set(flags.FlagFrom, args[0]); err != nil {
+				return err
+			}
 
 			clientCtx, err := client.GetClientTxContext(cmd)
 			if err != nil {

x/builder/keeper/auction.go

@@ -69,15 +69,25 @@ func (k Keeper) ValidateAuctionBid(ctx sdk.Context, bidder sdk.AccAddress, bid,
 			return err
 		}
 
+		if minBidIncrement.Denom != bid.Denom {
+			return fmt.Errorf("min bid increment denom (%s) does not match the bid denom (%s)", minBidIncrement, bid)
+		}
+
 		minBid := highestBid.Add(minBidIncrement)
 		if !bid.IsGTE(minBid) {
-			return fmt.Errorf("bid amount (%s) is less than the highest bid (%s) + min bid increment (%s)", bid, highestBid, minBidIncrement)
+			return fmt.Errorf(
+				"bid amount (%s) is less than the highest bid (%s) + min bid increment (%s); smallest acceptable bid is (%s)",
+				bid,
+				highestBid,
+				minBidIncrement,
+				minBid,
+			)
 		}
 	}
 
 	// ensure the bidder has enough funds to cover all the inclusion fees
-	balances := k.bankKeeper.GetAllBalances(ctx, bidder)
-	if !balances.IsAllGTE(sdk.NewCoins(bid)) {
+	balances := k.bankKeeper.GetBalance(ctx, bidder, bid.Denom)
+	if !balances.IsGTE(bid) {
 		return fmt.Errorf("insufficient funds to bid %s with balance %s", bid, balances)
 	}
 
@@ -114,15 +124,15 @@ func (k Keeper) ValidateAuctionBundle(bidder sdk.AccAddress, bundleSigners []map
 		// as long as all subsequent transactions are signed by the bidder.
 		if len(prevSigners) == 0 {
 			if seenBidder {
-				return fmt.Errorf("bundle contains transactions signed by multiple parties; possible front-running or sandwich attack")
+				return NewFrontRunningError()
 			}
 
 			seenBidder = true
 			prevSigners = map[string]struct{}{bidder.String(): {}}
 			filterSigners(prevSigners, txSigners)
 
 			if len(prevSigners) == 0 {
-				return fmt.Errorf("bundle contains transactions signed by multiple parties; possible front-running or sandwich attack")
+				return NewFrontRunningError()
 			}
 		}
 	}

x/builder/keeper/auction_test.go

@@ -15,7 +15,7 @@ func (suite *KeeperTestSuite) TestValidateBidInfo() {
 	var (
 		// Tx building variables
 		accounts = []testutils.Account{} // tracks the order of signers in the bundle
-		balance  = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+		balance  = sdk.NewCoin("foo", sdk.NewInt(10000))
 		bid      = sdk.NewCoin("foo", sdk.NewInt(1000))
 
 		// Auction params
@@ -48,7 +48,7 @@ func (suite *KeeperTestSuite) TestValidateBidInfo() {
 			"insufficient balance",
 			func() {
 				bid = sdk.NewCoin("foo", sdk.NewInt(1000))
-				balance = sdk.NewCoins()
+				balance = sdk.NewCoin("foo", sdk.NewInt(100))
 			},
 			false,
 		},
@@ -57,7 +57,7 @@ func (suite *KeeperTestSuite) TestValidateBidInfo() {
 			func() {
 				// reset the balance and bid to their original values
 				bid = sdk.NewCoin("foo", sdk.NewInt(1000))
-				balance = sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(10000)))
+				balance = sdk.NewCoin("foo", sdk.NewInt(10000))
 				accounts = testutils.RandomAccounts(rnd, int(maxBundleSize+1))
 			},
 			false,
@@ -141,6 +141,15 @@ func (suite *KeeperTestSuite) TestValidateBidInfo() {
 			},
 			false,
 		},
+		{
+			"min bid increment is different from bid denom", // THIS SHOULD NEVER HAPPEN
+			func() {
+				highestBid = sdk.NewCoin("foo", sdk.NewInt(500))
+				bid = sdk.NewCoin("foo", sdk.NewInt(1500))
+				minBidIncrement = sdk.NewCoin("foo2", sdk.NewInt(1000))
+			},
+			false,
+		},
 	}
 
 	for _, tc := range cases {
@@ -150,7 +159,7 @@ func (suite *KeeperTestSuite) TestValidateBidInfo() {
 			tc.malleate()
 
 			// Set up the new builder keeper with mocks customized for this test case
-			suite.bankKeeper.EXPECT().GetAllBalances(suite.ctx, bidder.Address).Return(balance).AnyTimes()
+			suite.bankKeeper.EXPECT().GetBalance(suite.ctx, bidder.Address, minBidIncrement.Denom).Return(balance).AnyTimes()
 			suite.bankKeeper.EXPECT().SendCoins(suite.ctx, bidder.Address, escrowAddress, reserveFee).Return(nil).AnyTimes()
 
 			suite.builderKeeper = keeper.NewKeeper(

x/builder/keeper/errors.go

@@ -0,0 +1,12 @@
+package keeper
+
+// FrontRunningError defines a custom error type for detecting front-running or sandwich attacks.
+type FrontRunningError struct{}
+
+func NewFrontRunningError() *FrontRunningError {
+	return &FrontRunningError{}
+}
+
+func (e FrontRunningError) Error() string {
+	return "bundle contains transactions signed by multiple parties; possible front-running or sandwich attack"
+}

x/builder/keeper/msg_server.go

@@ -33,31 +33,24 @@ func (m MsgServer) AuctionBid(goCtx context.Context, msg *types.MsgAuctionBid) (
 		return nil, err
 	}
 
-	// Ensure that the number of transactions is less than or equal to the maximum
-	// allowed.
-	maxBundleSize, err := m.GetMaxBundleSize(ctx)
+	params, err := m.GetParams(ctx)
 	if err != nil {
 		return nil, err
 	}
 
-	if uint32(len(msg.Transactions)) > maxBundleSize {
-		return nil, fmt.Errorf("the number of transactions in the bid is greater than the maximum allowed; expected <= %d, got %d", maxBundleSize, len(msg.Transactions))
+	if uint32(len(msg.Transactions)) > params.MaxBundleSize {
+		return nil, fmt.Errorf("the number of transactions in the bid is greater than the maximum allowed; expected <= %d, got %d", params.MaxBundleSize, len(msg.Transactions))
 	}
 
-	proposerFee, err := m.GetProposerFee(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	escrow, err := m.Keeper.GetEscrowAccount(ctx)
+	escrowAddress, err := sdk.AccAddressFromBech32(params.EscrowAccountAddress)
 	if err != nil {
 		return nil, err
 	}
 
 	var proposerReward sdk.Coins
-	if proposerFee.IsZero() {
+	if params.ProposerFee.IsZero() {
 		// send the entire bid to the escrow account when no proposer fee is set
-		if err := m.bankKeeper.SendCoins(ctx, bidder, escrow, sdk.NewCoins(msg.Bid)); err != nil {
+		if err := m.bankKeeper.SendCoins(ctx, bidder, escrowAddress, sdk.NewCoins(msg.Bid)); err != nil {
 			return nil, err
 		}
 	} else {
@@ -66,7 +59,7 @@ func (m MsgServer) AuctionBid(goCtx context.Context, msg *types.MsgAuctionBid) (
 
 		// determine the amount of the bid that goes to the (previous) proposer
 		bid := sdk.NewDecCoinsFromCoins(msg.Bid)
-		proposerReward, _ = bid.MulDecTruncate(proposerFee).TruncateDecimal()
+		proposerReward, _ = bid.MulDecTruncate(params.ProposerFee).TruncateDecimal()
 
 		if err := m.bankKeeper.SendCoins(ctx, bidder, sdk.AccAddress(prevProposer.GetOperator()), proposerReward); err != nil {
 			return nil, err
@@ -77,7 +70,7 @@ func (m MsgServer) AuctionBid(goCtx context.Context, msg *types.MsgAuctionBid) (
 		escrowTotal := bid.Sub(sdk.NewDecCoinsFromCoins(proposerReward...))
 		escrowReward, _ := escrowTotal.TruncateDecimal()
 
-		if err := m.bankKeeper.SendCoins(ctx, bidder, escrow, escrowReward); err != nil {
+		if err := m.bankKeeper.SendCoins(ctx, bidder, escrowAddress, escrowReward); err != nil {
 			return nil, err
 		}
 	}

x/builder/types/expected_keepers.go

@@ -13,7 +13,7 @@ type AccountKeeper interface {
 // BankKeeper defines the expected API contract for the x/bank module.
 type BankKeeper interface {
 	SendCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
-	GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins
+	GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
 }
 
 // DistributionKeeper defines the expected API contract for the x/distribution

x/builder/types/msgs_test.go

@@ -148,6 +148,19 @@ func TestMsgUpdateParams(t *testing.T) {
 			},
 			expectPass: false,
 		},
+		{
+			description: "invalid message with min bid increment equal to 0",
+			msg: types.MsgUpdateParams{
+				Authority: sdk.AccAddress([]byte("test")).String(),
+				Params: types.Params{
+					ProposerFee:          sdk.NewDec(1),
+					EscrowAccountAddress: sdk.AccAddress([]byte("test")).String(),
+					ReserveFee:           sdk.NewCoin("test", sdk.NewInt(100)),
+					MinBidIncrement:      sdk.NewCoin("test", sdk.NewInt(0)),
+				},
+			},
+			expectPass: false,
+		},
 	}
 
 	for _, tc := range cases {