[audit] fix: avoid reprocessing withdrawals (#20)

* fix: avoid re-processing withdrawals

1. check pending nonce and chain nonce before processing
2. check recently processed using local records before processing

* feat: update db types `BotDelegatedWithdrawal`

1. Rename L2ContractEvent to BotDelegatedWithdrawal
2. Add unique constraint idx_bot_delegated_withdrawals_transaction_hash_log_index_key
3. Add new field `InitiatedBlockNumber int64` to indicate the L2 number
   of initiated withdrawal transaction
3. Add new fields `ProvenTime *Time` and `FinalizedTime *Time` to
   indicate the local time of L1 proven transaction and finalized
   transaction
4. Modify the `FailureReason` to type `FailureReason *string`

* improve: compare timings of proven and finalized more precisely

1. Determine the proven timing based on the `L2OutputOracle.latestBlockNumber`
2. Determine the finalized timing based on the db `proven_time`

* bindings: update binding

* feat: manage nonce locally

* config: update bot contract

bot.testnet.toml

@@ -23,7 +23,7 @@ l2-output-oracle                = "0xff2394bb843012562f4349c6632a0ecb92fc8810"
 l1-cross-domain-messenger       = "0xd506952e78eecd5d4424b1990a0c99b1568e7c2c"
 
 [l2-standard-bridge-bot]
-contract-address                = "0x62fa6549e240076D466E1150ce587905c55f12F1"
+contract-address                = "0xE750d1f9180294473baCd960Ce5F9576eFBd70f2"
 log-filter-block-range = 1000
 
 # See https://github.com/bnb-chain/opbnb-bridge-tokens#opbnb-testnet-token-list

cmd/bot/run.go

@@ -54,9 +54,9 @@ func RunCommand(ctx *cli.Context) error {
 	if err != nil {
 		return fmt.Errorf("failed to migrate l2_scanned_blocks: %w", err)
 	}
-	err = db.AutoMigrate(&core.L2ContractEvent{})
+	err = db.AutoMigrate(&core.BotDelegatedWithdrawal{})
 	if err != nil {
-		return fmt.Errorf("failed to migrate l2_contract_events: %w", err)
+		return fmt.Errorf("failed to migrate withdrawals: %w", err)
 	}
 
 	l2ScannedBlock, err := queryL2ScannedBlock(db, cfg.L2StartingNumber)
@@ -76,37 +76,68 @@ func RunCommand(ctx *cli.Context) error {
 // and it will finalize the withdrawal when the challenge time window has passed.
 func ProcessBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, l1Client *core.ClientExt, l2Client *core.ClientExt, cfg core.Config) {
 	ticker := time.NewTicker(3 * time.Second)
+	pending := core.NewPendingTxsManager()
 	for {
 		select {
 		case <-ticker.C:
-			ProcessUnprovenBotDelegatedWithdrawals(ctx, log, db, l1Client, l2Client, cfg)
-			ProcessUnfinalizedBotDelegatedWithdrawals(ctx, log, db, l1Client, l2Client, cfg)
+			// In order to avoid re-processing the same withdrawal, we need to check if the pending nonce is
+			// the chain nonce. If they are not equal, it means that there are some pending transactions that
+			// been confirmed yet.
+			_, signerAddress, _ := cfg.SignerKeyPair()
+			if equal, err := isPendingAndChainNonceEqual(l1Client, signerAddress); err != nil {
+				log.Error("failed to check pending and chain nonce", "error", err)
+				continue
+			} else if !equal {
+				log.Info("pending nonce is not equal to chain nonce, skip processing")
+				continue
+			}
+
+			currentNonce, err := l1Client.NonceAt(ctx, *signerAddress, nil)
+			if err != nil {
+				log.Error("failed to get chain nonce", "error", err)
+				continue
+			}
+
+			ProcessUnprovenBotDelegatedWithdrawals(ctx, log, db, l1Client, l2Client, cfg, pending, &currentNonce)
+			ProcessUnfinalizedBotDelegatedWithdrawals(ctx, log, db, l1Client, l2Client, cfg, pending, &currentNonce)
 		case <-ctx.Done():
 			return
 		}
 	}
 }
 
-func ProcessUnprovenBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, l1Client *core.ClientExt, l2Client *core.ClientExt, cfg core.Config) {
+func ProcessUnprovenBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, l1Client *core.ClientExt, l2Client *core.ClientExt, cfg core.Config, pending *core.PendingTxnCheck, currentNonce *uint64) {
+	latestProposedNumber, err := core.L2OutputOracleLatestBlockNumber(cfg.L1Contracts.L2OutputOracleProxy, l1Client)
+	if err != nil {
+		log.Error("failed to get latest proposed block number", "error", err)
+		return
+	}
+
 	processor := core.NewProcessor(log, l1Client, l2Client, cfg)
 	limit := 1000
-	maxBlockTime := time.Now().Unix() - cfg.ProposeTimeWindow
 
-	unprovens := make([]core.L2ContractEvent, 0)
-	result := db.Order("id asc").Where("proven = false AND block_time < ? AND failure_reason IS NULL", maxBlockTime).Limit(limit).Find(&unprovens)
+	unprovens := make([]core.BotDelegatedWithdrawal, 0)
+	result := db.Order("id asc").Where("proven_time IS NULL AND initiated_block_number <= ? AND failure_reason IS NULL", latestProposedNumber.Uint64()).Limit(limit).Find(&unprovens)
 	if result.Error != nil {
-		log.Error("failed to query l2_contract_events", "error", result.Error)
+		log.Error("failed to query withdrawals", "error", result.Error)
 		return
 	}
 
+	pending.Prune(*currentNonce)
 	for _, unproven := range unprovens {
-		err := processor.ProveWithdrawalTransaction(ctx, &unproven)
+		// Avoid re-processing the same withdrawal
+		if pending.IsPendingTxn(unproven.ID) {
+			continue
+		}
+
+		now := time.Now()
+		err := processor.ProveWithdrawalTransaction(ctx, &unproven, *currentNonce)
 		if err != nil {
 			if strings.Contains(err.Error(), "OptimismPortal: withdrawal hash has already been proven") {
 				// The withdrawal has already proven, mark it
-				result := db.Model(&unproven).Update("proven", true)
+				result := db.Model(&unproven).Update("proven_time", now)
 				if result.Error != nil {
-					log.Error("failed to update proven l2_contract_events", "error", result.Error)
+					log.Error("failed to update proven withdrawals", "error", result.Error)
 				}
 			} else if strings.Contains(err.Error(), "L2OutputOracle: cannot get output for a block that has not been proposed") {
 				// Since the unproven withdrawals are sorted by the on-chain order, we can break here because we know
@@ -116,37 +147,48 @@ func ProcessUnprovenBotDelegatedWithdrawals(ctx context.Context, log log.Logger,
 				// Proven transaction reverted, mark it with the failure reason
 				result := db.Model(&unproven).Update("failure_reason", err.Error())
 				if result.Error != nil {
-					log.Error("failed to update failure reason of l2_contract_events", "error", result.Error)
+					log.Error("failed to update failure reason of withdrawals", "error", result.Error)
 				}
 			} else {
 				// non-revert error, stop processing the subsequent withdrawals
 				log.Error("ProveWithdrawalTransaction", "non-revert error", err.Error())
 				return
 			}
+		} else {
+			pending.AddPendingTxn(unproven.ID, *currentNonce)
+			*currentNonce = *currentNonce + 1
 		}
 	}
 }
 
-func ProcessUnfinalizedBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, l1Client *core.ClientExt, l2Client *core.ClientExt, cfg core.Config) {
+func ProcessUnfinalizedBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, l1Client *core.ClientExt, l2Client *core.ClientExt, cfg core.Config, pending *core.PendingTxnCheck, currentNonce *uint64) {
 	processor := core.NewProcessor(log, l1Client, l2Client, cfg)
 	limit := 1000
-	maxBlockTime := time.Now().Unix() - cfg.ChallengeTimeWindow
 
-	unfinalizeds := make([]core.L2ContractEvent, 0)
-	result := db.Order("block_time asc").Where("proven = true AND finalized = false AND block_time < ? AND failure_reason IS NULL", maxBlockTime).Limit(limit).Find(&unfinalizeds)
+	now := time.Now()
+	maxProvenTime := now.Add(-time.Duration(cfg.ChallengeTimeWindow) * time.Second)
+
+	unfinalizeds := make([]core.BotDelegatedWithdrawal, 0)
+	result := db.Order("id asc").Where("finalized_time IS NULL AND proven_time IS NOT NULL AND proven_time < ? AND failure_reason IS NULL", maxProvenTime).Limit(limit).Find(&unfinalizeds)
 	if result.Error != nil {
-		log.Error("failed to query l2_contract_events", "error", result.Error)
+		log.Error("failed to query withdrawals", "error", result.Error)
 		return
 	}
 
+	pending.Prune(*currentNonce)
 	for _, unfinalized := range unfinalizeds {
+		// In order to avoid re-processing the same withdrawal
+		if pending.IsPendingTxn(unfinalized.ID) {
+			continue
+		}
+
 		err := processor.FinalizeMessage(ctx, &unfinalized)
 		if err != nil {
 			if strings.Contains(err.Error(), "OptimismPortal: withdrawal has already been finalized") {
 				// The withdrawal has already finalized, mark it
-				result := db.Model(&unfinalized).Update("finalized", true)
+				result := db.Model(&unfinalized).Update("finalized_time", now)
 				if result.Error != nil {
-					log.Error("failed to update finalized l2_contract_events", "error", result.Error)
+					log.Error("failed to update finalized withdrawals", "error", result.Error)
 				}
 			} else if strings.Contains(err.Error(), "OptimismPortal: withdrawal has not been proven yet") {
 				log.Error("detected a unproven withdrawal when send finalized transaction", "withdrawal", unfinalized)
@@ -158,13 +200,16 @@ func ProcessUnfinalizedBotDelegatedWithdrawals(ctx context.Context, log log.Logg
 				// Finalized transaction reverted, mark it with the failure reason
 				result := db.Model(&unfinalized).Update("failure_reason", err.Error())
 				if result.Error != nil {
-					log.Error("failed to update failure reason of l2_contract_events", "error", result.Error)
+					log.Error("failed to update failure reason of withdrawals", "error", result.Error)
 				}
 			} else {
 				// non-revert error, stop processing the subsequent withdrawals
 				log.Error("FinalizedMessage", "non-revert error", err.Error())
 				return
 			}
+		} else {
+			pending.AddPendingTxn(unfinalized.ID, *currentNonce)
+			*currentNonce = *currentNonce + 1
 		}
 	}
 }
@@ -178,13 +223,10 @@ func storeLogs(db *gorm.DB, client *core.ClientExt, logs []types.Log) error {
 			return err
 		}
 
-		event := core.L2ContractEvent{
-			BlockTime:       int64(header.Time),
-			BlockHash:       vLog.BlockHash.Hex(),
-			ContractAddress: vLog.Address.Hex(),
-			TransactionHash: vLog.TxHash.Hex(),
-			LogIndex:        int(vLog.Index),
-			EventSignature:  vLog.Topics[0].Hex(),
+		event := core.BotDelegatedWithdrawal{
+			TransactionHash:      vLog.TxHash.Hex(),
+			LogIndex:             int(vLog.Index),
+			InitiatedBlockNumber: int64(header.Number.Uint64()),
 		}
 
 		deduped := db.Clauses(
@@ -200,9 +242,9 @@ func storeLogs(db *gorm.DB, client *core.ClientExt, logs []types.Log) error {
 }
 
 // WatchBotDelegatedWithdrawals watches for new bot-delegated withdrawals and stores them in the database.
-func WatchBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, client *core.ClientExt, l2ScannedBlock *core.L2ScannedBlock, cfg core.Config) {
+func WatchBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.DB, client *core.ClientExt, l2StartingBlock *core.L2ScannedBlock, cfg core.Config) {
 	timer := time.NewTimer(0)
-	fromBlockNumber := big.NewInt(l2ScannedBlock.Number)
+	fromBlockNumber := big.NewInt(l2StartingBlock.Number)
 
 	for {
 		select {
@@ -246,8 +288,8 @@ func WatchBotDelegatedWithdrawals(ctx context.Context, log log.Logger, db *gorm.
 			}
 		}
 
-		l2ScannedBlock.Number = toBlockNumber.Int64()
-		result := db.Where("number >= 0").Updates(l2ScannedBlock)
+		l2StartingBlock.Number = toBlockNumber.Int64()
+		result := db.Save(l2StartingBlock)
 		if result.Error != nil {
 			log.Error("update l2_scanned_blocks", "error", result.Error)
 		}
@@ -297,7 +339,7 @@ func connect(log log.Logger, dbConfig config.DBConfig) (*gorm.DB, error) {
 
 // queryL2ScannedBlock queries the l2_scanned_blocks table for the last scanned block
 func queryL2ScannedBlock(db *gorm.DB, l2StartingNumber int64) (*core.L2ScannedBlock, error) {
-    l2ScannedBlock := core.L2ScannedBlock{Number: l2StartingNumber}
+	l2ScannedBlock := core.L2ScannedBlock{Number: l2StartingNumber}
 	result := db.Order("number desc").Last(&l2ScannedBlock)
 	if result.Error != nil {
 		if errors.Is(result.Error, gorm.ErrRecordNotFound) {
@@ -308,3 +350,18 @@ func queryL2ScannedBlock(db *gorm.DB, l2StartingNumber int64) (*core.L2ScannedBl
 	}
 	return &l2ScannedBlock, nil
 }
+
+// isPendingAndChainNonceEqual checks if the pending nonce and the chain nonce are equal.
+func isPendingAndChainNonceEqual(l1Client *core.ClientExt, address *common.Address) (bool, error) {
+	pendingNonce, err := l1Client.PendingNonceAt(context.Background(), *address)
+	if err != nil {
+		return false, fmt.Errorf("failed to get pending nonce: %w", err)
+	}
+
+	latestNonce, err := l1Client.NonceAt(context.Background(), *address, nil)
+	if err != nil {
+		return false, fmt.Errorf("failed to get latest nonce: %w", err)
+	}
+
+	return pendingNonce == latestNonce, nil
+}

core/bindings.go

@@ -1,6 +1,9 @@
 package core
 
 import (
+	"math/big"
+
+	"github.com/ethereum-optimism/optimism/op-bindings/bindings"
 	"github.com/ethereum/go-ethereum/common"
 	"golang.org/x/crypto/sha3"
 )
@@ -17,3 +20,13 @@ func WithdrawToEventSig() common.Hash {
 	eventSignatureHash := keccak256.Sum(nil)
 	return common.BytesToHash(eventSignatureHash)
 }
+
+// L2OutputOracleLatestBlockNumber calls the "latestBlockNumber" function on the L2OutputOracle contract at the given address.
+func L2OutputOracleLatestBlockNumber(address common.Address, l1Client *ClientExt) (*big.Int, error) {
+	caller, err := bindings.NewL2OutputOracleCaller(address, l1Client)
+	if err != nil {
+		return nil, err
+	}
+
+	return caller.LatestBlockNumber(nil)
+}

core/pending_txn_check.go

@@ -0,0 +1,30 @@
+package core
+
+type PendingTxnCheck struct {
+	inner map[uint]uint64 // #{withdrawalId=>nonce}
+}
+
+// NewPendingTxsManager creates a new PendingTxnCheck
+func NewPendingTxsManager() *PendingTxnCheck {
+	return &PendingTxnCheck{inner: make(map[uint]uint64)}
+}
+
+// IsPendingTxn checks whether there is pending transaction for the specific event id.
+func (c *PendingTxnCheck) IsPendingTxn(id uint) bool {
+	_, ok := c.inner[id]
+	return ok
+}
+
+// AddPendingTxn adds a pending item.
+func (c *PendingTxnCheck) AddPendingTxn(id uint, nonce uint64) {
+	c.inner[id] = nonce
+}
+
+// Prune removes the transactions with staled nonce.
+func (c *PendingTxnCheck) Prune(chainNonce uint64) {
+	for id, nonce := range c.inner {
+		if nonce <= chainNonce {
+			delete(c.inner, id)
+		}
+	}
+}

core/processor.go

@@ -48,7 +48,7 @@ func NewProcessor(
 	return &Processor{log, l1Client, l2Client, cfg, l2Contracts, whitelistL2TokenMap}
 }
 
-func (b *Processor) toWithdrawal(botDelegatedWithdrawToEvent *L2ContractEvent, receipt *types.Receipt) (*bindings.TypesWithdrawalTransaction, error) {
+func (b *Processor) toWithdrawal(botDelegatedWithdrawToEvent *BotDelegatedWithdrawal, receipt *types.Receipt) (*bindings.TypesWithdrawalTransaction, error) {
 	// Events flow:
 	//
 	// event[i-5]: WithdrawalInitiated
@@ -82,7 +82,7 @@ func (b *Processor) toWithdrawal(botDelegatedWithdrawToEvent *L2ContractEvent, r
 	return withdrawalTx, nil
 }
 
-func (b *Processor) ProveWithdrawalTransaction(ctx context.Context, botDelegatedWithdrawToEvent *L2ContractEvent) error {
+func (b *Processor) ProveWithdrawalTransaction(ctx context.Context, botDelegatedWithdrawToEvent *BotDelegatedWithdrawal, nonce uint64) error {
 	receipt, err := b.L2Client.TransactionReceipt(ctx, common.HexToHash(botDelegatedWithdrawToEvent.TransactionHash))
 	if err != nil {
 		return err
@@ -170,6 +170,7 @@ func (b *Processor) ProveWithdrawalTransaction(ctx context.Context, botDelegated
 			Signer: func(address common.Address, tx *types.Transaction) (*types.Transaction, error) {
 				return types.SignTx(tx, types.NewEIP155Signer(l1ChainId), signerPrivkey)
 			},
+			Nonce: big.NewInt(int64(nonce)),
 		},
 		*withdrawalTx,
 		l2OutputIndex,
@@ -185,7 +186,7 @@ func (b *Processor) ProveWithdrawalTransaction(ctx context.Context, botDelegated
 }
 
 // FinalizeMessage https://github.com/ethereum-optimism/optimism/blob/d90e7818de894f0bc93ae7b449b9049416bda370/packages/sdk/src/cross-chain-messenger.ts#L1611
-func (b *Processor) FinalizeMessage(ctx context.Context, botDelegatedWithdrawToEvent *L2ContractEvent) error {
+func (b *Processor) FinalizeMessage(ctx context.Context, botDelegatedWithdrawToEvent *BotDelegatedWithdrawal) error {
 	receipt, err := b.L2Client.TransactionReceipt(ctx, common.HexToHash(botDelegatedWithdrawToEvent.TransactionHash))
 	if err != nil {
 		return err
@@ -529,7 +530,7 @@ func (b *Processor) toLowLevelMessage(
 	return &withdrawalTx, nil
 }
 
-func (b *Processor) CheckByFilterOptions(botDelegatedWithdrawToEvent *L2ContractEvent, receipt *types.Receipt) error {
+func (b *Processor) CheckByFilterOptions(botDelegatedWithdrawToEvent *BotDelegatedWithdrawal, receipt *types.Receipt) error {
 	L2StandardBridgeBotAbi, _ := bindings2.L2StandardBridgeBotMetaData.GetAbi()
 	withdrawToEvent := bindings2.L2StandardBridgeBotWithdrawTo{}
 	indexedArgs := func(arguments abi.Arguments) abi.Arguments {

core/types.go

@@ -1,18 +1,28 @@
 package core
 
+import "time"
+
 type L2ScannedBlock struct {
 	Number int64 `gorm:"type:integer;primarykey"`
 }
 
-type L2ContractEvent struct {
-	ID              uint   `gorm:"primarykey"`
-	BlockTime       int64  `gorm:"type:integer;not null;index:idx_l2_contract_events_block_time"`
-	BlockHash       string `gorm:"type:varchar(256);not null;uniqueIndex:idx_l2_contract_events_block_hash_log_index_key,priority:1;"`
-	ContractAddress string `gorm:"type:varchar(256);not null"`
-	TransactionHash string `gorm:"type:varchar(256);not null"`
-	LogIndex        int    `gorm:"type:integer;not null;uniqueIndex:idx_l2_contract_events_block_hash_log_index_key,priority:2"`
-	EventSignature  string `gorm:"type:varchar(256);not null"`
-	Proven          bool   `gorm:"type:boolean;not null;default:false"`
-	Finalized       bool   `gorm:"type:boolean;not null;default:false"`
-	FailureReason   string `gorm:"type:text"`
+type BotDelegatedWithdrawal struct {
+	// ID is the incrementing primary key.
+	ID uint `gorm:"primarykey"`
+
+	// TransactionHash and LogIndex are the L2 transaction hash and log index of the withdrawal event.
+	TransactionHash string `gorm:"type:varchar(256);not null;uniqueIndex:idx_bot_delegated_withdrawals_transaction_hash_log_index_key,priority:1"`
+	LogIndex        int    `gorm:"type:integer;not null;uniqueIndex:idx_bot_delegated_withdrawals_transaction_hash_log_index_key,priority:2"`
+
+	// InitiatedBlockNumber is the l2 block number at which the withdrawal was initiated on L2.
+	InitiatedBlockNumber int64 `gorm:"type:integer;not null;index:idx_withdrawals_initiated_block_number"`
+
+	// ProvenTime is the local time at which the withdrawal was proven on L1. NULL if not yet proven.
+	ProvenTime *time.Time `gorm:"type:datetime;index:idx_withdrawals_proven_time"`
+
+	// FinalizedTime is the local time at which the withdrawal was finalized on L1. NULL if not yet finalized.
+	FinalizedTime *time.Time `gorm:"type:datetime;index:idx_withdrawals_finalized_time"`
+
+	// FailureReason is the reason for the withdrawal failure, including sending transaction error and off-chain configured filter error. NULL if not yet failed.
+	FailureReason *string `gorm:"type:text"`
 }