fix: audit (#107)

* do not create account when it is exists * reclaim and burn receiver funds * fix to return all validators at val state updates * fix to store by cons addr * fix emit reason event only at failure * disallow "hook" at validate
initia-labs · Sep 19, 2024 · 4d593d1 · 4d593d1
1 parent 0024749
commit 4d593d1
Show file tree

Hide file tree

Showing 9 changed files with 125 additions and 35 deletions.
diff --git a/x/opchild/keeper/deposit.go b/x/opchild/keeper/deposit.go
@@ -59,8 +59,11 @@ func (k Keeper) handleBridgeHook(ctx sdk.Context, data []byte) (success bool, re
 func (ms MsgServer) safeDepositToken(ctx context.Context, toAddr sdk.AccAddress, coins sdk.Coins) (success bool, reason string) {
 	// if coin is zero, just create an account
 	if coins.IsZero() {
-		newAcc := ms.authKeeper.NewAccountWithAddress(ctx, toAddr)
-		ms.authKeeper.SetAccount(ctx, newAcc)
+		if !ms.authKeeper.HasAccount(ctx, toAddr) {
+			newAcc := ms.authKeeper.NewAccountWithAddress(ctx, toAddr)
+			ms.authKeeper.SetAccount(ctx, newAcc)
+		}
+
 		return true, ""
 	}
 

diff --git a/x/opchild/keeper/executor_change.go b/x/opchild/keeper/executor_change.go
@@ -71,6 +71,9 @@ func (k Keeper) ChangeExecutor(ctx context.Context, plan types.ExecutorChangePla
 	if err := k.SetValidator(ctx, plan.NextValidator); err != nil {
 		return err
 	}
+	if err = k.SetValidatorByConsAddr(ctx, plan.NextValidator); err != nil {
+		return err
+	}
 
 	params, err := k.GetParams(ctx)
 	if err != nil {

diff --git a/x/opchild/keeper/executor_change_test.go b/x/opchild/keeper/executor_change_test.go
@@ -9,6 +9,8 @@ import (
 	"testing"
 
 	"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
+	testutilsims "github.com/cosmos/cosmos-sdk/testutil/sims"
+
 	"github.com/initia-labs/OPinit/x/opchild/types"
 	"github.com/stretchr/testify/require"
 )
@@ -53,3 +55,65 @@ func Test_RegisterExecutorChangePlan(t *testing.T) {
 		Info:          info,
 	}, input.OPChildKeeper.ExecutorChangePlans[height.Uint64()])
 }
+
+func Test_ExecuteChangePlan(t *testing.T) {
+	ctx, input := createDefaultTestInput(t)
+
+	valPubKeys := testutilsims.CreateTestPubKeys(2)
+	val1, err := types.NewValidator(valAddrs[1], valPubKeys[0], "validator1")
+	require.NoError(t, err)
+
+	val2, err := types.NewValidator(valAddrs[2], valPubKeys[1], "validator2")
+	require.NoError(t, err)
+
+	// set validators
+	require.NoError(t, input.OPChildKeeper.SetValidator(ctx, val1))
+	require.NoError(t, input.OPChildKeeper.SetValidatorByConsAddr(ctx, val1))
+	require.NoError(t, input.OPChildKeeper.SetValidator(ctx, val2))
+	require.NoError(t, input.OPChildKeeper.SetValidatorByConsAddr(ctx, val2))
+
+	// Arguments
+	l1ProposalID, err := rand.Int(rand.Reader, big.NewInt(math.MaxInt64))
+	require.NoError(t, err)
+	height, err := rand.Int(rand.Reader, big.NewInt(math.MaxInt64))
+	require.NoError(t, err)
+	nextValAddr := valAddrsStr[0]
+	nextExecutorAddr := []string{addrsStr[0], addrsStr[1]}
+	consensusPubKey := "l7aqGv+Zjbm0rallfqfqz+3iN31iOmgJCafWV5pGs6o="
+	moniker := "moniker"
+	info := "info"
+
+	err = input.OPChildKeeper.RegisterExecutorChangePlan(
+		l1ProposalID.Uint64(), height.Uint64(), nextValAddr,
+		moniker,
+		fmt.Sprintf(`{"@type":"/cosmos.crypto.ed25519.PubKey","key":"%s"}`, consensusPubKey),
+		info,
+		nextExecutorAddr,
+	)
+	require.NoError(t, err)
+	require.Len(t, input.OPChildKeeper.ExecutorChangePlans, 1)
+
+	err = input.OPChildKeeper.ChangeExecutor(ctx, input.OPChildKeeper.ExecutorChangePlans[height.Uint64()])
+	require.NoError(t, err)
+
+	// Check if the validator has been updated
+	validator, found := input.OPChildKeeper.GetValidator(ctx, valAddrs[0])
+	require.True(t, found)
+	require.Equal(t, moniker, validator.GetMoniker())
+	require.Equal(t, int64(1), validator.ConsPower)
+
+	consAddr, err := validator.GetConsAddr()
+	require.NoError(t, err)
+	v := input.OPChildKeeper.ValidatorByConsAddr(ctx, consAddr)
+	require.Equal(t, moniker, v.GetMoniker())
+	require.Equal(t, int64(1), v.GetConsensusPower())
+
+	// Check if the old validators have been updated
+	validator, found = input.OPChildKeeper.GetValidator(ctx, valAddrs[1])
+	require.True(t, found)
+	require.Equal(t, int64(0), validator.ConsPower)
+
+	validator, found = input.OPChildKeeper.GetValidator(ctx, valAddrs[2])
+	require.True(t, found)
+	require.Equal(t, int64(0), validator.ConsPower)
+}
diff --git a/x/opchild/keeper/msg_server.go b/x/opchild/keeper/msg_server.go
@@ -395,15 +395,15 @@ func (ms MsgServer) FinalizeTokenDeposit(ctx context.Context, req *types.MsgFina
 	}
 
 	// deposit token
-	var success bool
+	var depositSuccess bool
 	var reason string
 	toAddr, err := ms.authKeeper.AddressCodec().StringToBytes(req.To)
 	if err != nil {
-		success = false
+		depositSuccess = false
 		reason = fmt.Sprintf("failed to convert recipient address: %s", err)
 	} else {
 		// rollback if the deposit is failed
-		success, reason = ms.safeDepositToken(ctx, toAddr, sdk.NewCoins(coin))
+		depositSuccess, reason = ms.safeDepositToken(ctx, toAddr, sdk.NewCoins(coin))
 	}
 
 	// updae l1 sequence
@@ -434,22 +434,39 @@ func (ms MsgServer) FinalizeTokenDeposit(ctx context.Context, req *types.MsgFina
 		sdk.NewAttribute(types.AttributeKeyBaseDenom, req.BaseDenom),
 		sdk.NewAttribute(types.AttributeKeyAmount, coin.Amount.String()),
 		sdk.NewAttribute(types.AttributeKeyFinalizeHeight, strconv.FormatUint(req.Height, 10)),
-		sdk.NewAttribute(types.AttributeKeySuccess, strconv.FormatBool(success)),
-		sdk.NewAttribute(types.AttributeKeyReason, reason),
 	)
 
 	// if the deposit is successful and the data is not empty, execute the hook
-	if success && len(req.Data) > 0 {
-		success, reason := ms.handleBridgeHook(sdkCtx, req.Data)
-		event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeyHookSuccess, strconv.FormatBool(success)))
-		event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeyHookReason, reason))
+	hookSuccess := true
+	if depositSuccess && len(req.Data) > 0 {
+		hookSuccess, reason = ms.handleBridgeHook(sdkCtx, req.Data)
+		event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeySuccess, strconv.FormatBool(hookSuccess)))
+		if !hookSuccess {
+			event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeyReason, "hook failed; "+reason))
+		}
+	} else {
+		event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeySuccess, strconv.FormatBool(depositSuccess)))
+		if !depositSuccess {
+			event = event.AppendAttributes(sdk.NewAttribute(types.AttributeKeyReason, "deposit failed; "+reason))
+		}
 	}
 
 	// emit deposit event
 	sdkCtx.EventManager().EmitEvent(event)
 
 	// if the deposit is failed, initate a withdrawal
-	if !success {
+	if !depositSuccess || !hookSuccess {
+		if depositSuccess {
+			// reclaim and burn coins
+			burnCoins := sdk.NewCoins(coin)
+			if err := ms.bankKeeper.SendCoinsFromAccountToModule(ctx, toAddr, types.ModuleName, burnCoins); err != nil {
+				return nil, err
+			}
+			if err := ms.bankKeeper.BurnCoins(ctx, types.ModuleName, burnCoins); err != nil {
+				return nil, err
+			}
+		}
+
 		l2Sequence, err := ms.IncreaseNextL2Sequence(ctx)
 		if err != nil {
 			return nil, err
@@ -482,11 +499,8 @@ func (ms MsgServer) InitiateTokenWithdrawal(ctx context.Context, req *types.MsgI
 	}
 
 	// send coins to the module account only if the amount is positive
-	// - pending deposits are already accounted for
-	if coin.IsPositive() {
-		if err := ms.bankKeeper.SendCoinsFromAccountToModule(ctx, senderAddr, types.ModuleName, sdk.NewCoins(coin)); err != nil {
-			return nil, err
-		}
+	if err := ms.bankKeeper.SendCoinsFromAccountToModule(ctx, senderAddr, types.ModuleName, burnCoins); err != nil {
+		return nil, err
 	}
 
 	// burn withdrawn coins from the module account

diff --git a/x/opchild/keeper/msg_server_test.go b/x/opchild/keeper/msg_server_test.go
@@ -413,13 +413,22 @@ func Test_MsgServer_Deposit_ToModuleAccount(t *testing.T) {
 	_, err := ms.FinalizeTokenDeposit(ctx, msg)
 	require.NoError(t, err)
 
+	for _, event := range sdk.UnwrapSDKContext(ctx).EventManager().Events() {
+		if event.Type == types.EventTypeFinalizeTokenDeposit {
+			attrIdx := slices.Index(event.Attributes, sdk.NewAttribute(types.AttributeKeySuccess, "false").ToKVPair())
+			require.Positive(t, attrIdx)
+			require.Equal(t, event.Attributes[attrIdx+1].Key, types.AttributeKeyReason)
+			require.Contains(t, event.Attributes[attrIdx+1].Value, "deposit failed;")
+		}
+	}
+
 	afterToBalance := input.BankKeeper.GetBalance(ctx, addrs[1], denom)
 	require.Equal(t, math.ZeroInt(), afterToBalance.Amount)
 
 	afterModuleBalance := input.BankKeeper.GetBalance(ctx, opchildModuleAddress, denom)
 	require.True(t, afterModuleBalance.Amount.IsZero())
 
-	// token withdrawal inititated
+	// token withdrawal initiated
 	events := sdk.UnwrapSDKContext(ctx).EventManager().Events()
 	lastEvent := events[len(events)-1]
 	require.Equal(t, sdk.NewEvent(
@@ -491,7 +500,7 @@ func Test_MsgServer_Deposit_HookSuccess(t *testing.T) {
 
 	for _, event := range sdk.UnwrapSDKContext(ctx).EventManager().Events() {
 		if event.Type == types.EventTypeFinalizeTokenDeposit {
-			require.True(t, slices.Contains(event.Attributes, sdk.NewAttribute(types.AttributeKeyHookSuccess, "true").ToKVPair()))
+			require.True(t, slices.Contains(event.Attributes, sdk.NewAttribute(types.AttributeKeySuccess, "true").ToKVPair()))
 		}
 	}
 
@@ -534,13 +543,20 @@ func Test_MsgServer_Deposit_HookFail(t *testing.T) {
 
 	for _, event := range sdk.UnwrapSDKContext(ctx).EventManager().Events() {
 		if event.Type == types.EventTypeFinalizeTokenDeposit {
-			require.True(t, slices.Contains(event.Attributes, sdk.NewAttribute(types.AttributeKeyHookSuccess, "false").ToKVPair()))
+			attrIdx := slices.Index(event.Attributes, sdk.NewAttribute(types.AttributeKeySuccess, "false").ToKVPair())
+			require.Positive(t, attrIdx)
+			require.Equal(t, event.Attributes[attrIdx+1].Key, types.AttributeKeyReason)
+			require.Contains(t, event.Attributes[attrIdx+1].Value, "hook failed;")
 		}
 	}
 
 	// check addrs[2] balance
 	afterBalance := input.BankKeeper.GetBalance(ctx, addrs[2], denom)
 	require.Equal(t, math.NewInt(0), afterBalance.Amount)
+
+	// check receiver has no balance
+	afterBalance = input.BankKeeper.GetBalance(ctx, addr, denom)
+	require.Equal(t, math.NewInt(0), afterBalance.Amount)
 }
 
 func Test_MsgServer_UpdateOracle(t *testing.T) {

diff --git a/x/opchild/keeper/val_state_change.go b/x/opchild/keeper/val_state_change.go
@@ -31,12 +31,7 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) ([]abci.V
 	}
 
 	updates := []abci.ValidatorUpdate{}
-	maxValidators, err := k.MaxValidators(ctx)
-	if err != nil {
-		return nil, err
-	}
-
-	validators, err := k.GetValidators(ctx, maxValidators)
+	validators, err := k.GetAllValidators(ctx)
 	if err != nil {
 		return nil, err
 	}

diff --git a/x/opchild/types/events.go b/x/opchild/types/events.go
@@ -28,8 +28,6 @@ const (
 	AttributeKeyMetadata       = "metadata"
 	AttributeKeyL1Sequence     = "l1_sequence"
 	AttributeKeyFinalizeHeight = "finalize_height"
-	AttributeKeyHookSuccess    = "hook_success"
-	AttributeKeyHookReason     = "hook_reason"
 	AttributeKeyFrom           = "from"
 	AttributeKeyTo             = "to"
 	AttributeKeyL2Sequence     = "l2_sequence"

diff --git a/x/opchild/types/tx.go b/x/opchild/types/tx.go
@@ -171,7 +171,7 @@ func (msg MsgInitiateTokenWithdrawal) Validate(ac address.Codec) error {
 		return sdkerrors.ErrInvalidAddress.Wrap("to address cannot be empty")
 	}
 
-	if !msg.Amount.IsValid() || msg.Amount.IsZero() {
+	if !msg.Amount.IsValid() || !msg.Amount.IsPositive() {
 		return ErrInvalidAmount
 	}
 
@@ -251,11 +251,8 @@ func (msg MsgFinalizeTokenDeposit) Validate(ac address.Codec) error {
 		return sdkerrors.ErrInvalidAddress.Wrap("from address cannot be empty")
 	}
 
-	// allow hook as a special address
-	if msg.To != "hook" {
-		if _, err := ac.StringToBytes(msg.To); err != nil {
-			return err
-		}
+	if _, err := ac.StringToBytes(msg.To); err != nil {
+		return err
 	}
 
 	// allow zero amount

diff --git a/x/ophost/keeper/msg_server_test.go b/x/ophost/keeper/msg_server_test.go
@@ -163,7 +163,7 @@ func Test_InitiateTokenDeposit(t *testing.T) {
 	input.Faucet.Fund(ctx, addrs[1], amount)
 	_, err = ms.InitiateTokenDeposit(
 		ctx,
-		types.NewMsgInitiateTokenDeposit(addrsStr[1], 1, "hook", amount, []byte("messages")),
+		types.NewMsgInitiateTokenDeposit(addrsStr[1], 1, "l2_addr", amount, []byte("messages")),
 	)
 	require.NoError(t, err)
 	require.True(t, input.BankKeeper.GetBalance(ctx, addrs[1], sdk.DefaultBondDenom).IsZero())