Missing Requirement Length Lead to Out of Bounds Error and Panic

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/evm_hooks.go#L271
https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/pkg/contracts/zevm/zrc20.sol/zrc20.go#L1793

Vulnerability details

Impact

The function ParseZRC20WithdrawalEvent() is used the parse the logs emitted by a withdrawal transaction.
However, there is no check on the length of the Topics logs by the function ParseWithdrawal() thus an attacker can craft an empty logs using the opcode LOG0 directly or using a anonymous event such as:

event Hello(address who) anonymous;

As the logs is transformed in an array.

However, as the array will be empty if we try to access to the first index this will panic in Golang.

File: https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/node/x/crosschain/keeper/evm_hooks.go#L271

// ParseZRC20WithdrawalEvent tries extracting Withdrawal event from registered ZRC20 contract;
// returns error if the log entry is not a Withdrawal event, or is not emitted from a
// registered ZRC20 contract
func (k Keeper) ParseZRC20WithdrawalEvent(ctx sdk.Context, log ethtypes.Log) (*zrc20.ZRC20Withdrawal, error) {
	zrc20ZEVM, err := zrc20.NewZRC20Filterer(log.Address, bind.ContractFilterer(nil))
	if err != nil {
		return nil, err
	}
	event, err := zrc20ZEVM.ParseWithdrawal(log)
	if err != nil {
		return nil, err
	}

	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
}

File: https://github.com/code-423n4/2023-11-zetachain/blob/b237708ed5e86f12c4bddabddfd42f001e81941a/repos/protocol-contracts/pkg/contracts/zevm/zrc20.sol/zrc20.go#L1793

// ParseWithdrawal is a log parse operation binding the contract event 0x9ffbffc04a397460ee1dbe8c9503e098090567d6b7f4b3c02a8617d800b6d955.
//
// Solidity: event Withdrawal(address indexed from, bytes to, uint256 value, uint256 gasfee, uint256 protocolFlatFee)
func (_ZRC20 *ZRC20Filterer) ParseWithdrawal(log types.Log) (*ZRC20Withdrawal, error) {
	event := new(ZRC20Withdrawal)
	if err := _ZRC20.contract.UnpackLog(event, "Withdrawal", log); err != nil { //Unpack logs is not checking also the length of the topics thus leading to a panic.
		return nil, err
	}
	event.Raw = log
	return event, nil
}

The issue lies inside the function UnpackLog() as the code is not checking the size of the log.Topics if the length of the array (log.Topics) is equal to 0 (meaning the logs are empty) then calling the index [0] will cause a panic.

// UnpackLog unpacks a retrieved log into the provided output structure.
func (c *BoundContract) UnpackLog(out interface{}, event string, log types.Log) error {	
 if log.Topics[0] != c.abi.Events[event].ID { //No check that the log.Topics[0] is existing, thus the code will call at the index [0] and panic..
		return fmt.Errorf("event signature mismatch")
	}
	if len(log.Data) > 0 {
		if err := c.abi.UnpackIntoInterface(out, event, log.Data); err != nil {
			return err
		}
	}
	var indexed abi.Arguments
	for _, arg := range c.abi.Events[event].Inputs {
		if arg.Indexed {
			indexed = append(indexed, arg)
		}
	}
	return abi.ParseTopics(out, indexed, log.Topics[1:])
}

As the RPC client is behind a recover() block, then panics are naturally handled in Golang.
That means that in this case a panic will not lead to having the client crash abruptly.

It just means that the operation submitted by the RPC will halt at this point and simply exit. But this property may leave the execution in a transient state.

In short, when the code loops over each log, it may process the first valid ones, but once it stumbles across the anonymous log, then the execution will halt and any log that should have been processed right after this won’t be processed and will never be.

This is an interesting property that could be exploited by a malicious user.

Note that this issue has been fixed in an earlier version of Go-Ethereum at the following PR: ethereum/go-ethereum#26920

Proof of Concept

Let’s think about a list of logs of a given transaction that could put users in trouble.

[1. ZetaSent(...), 2. anonymous, 3. Withdrawal(...)]

What would happen in this case?

1. ZetaSent → mint/transfer ZETA tokens on the receiving chain.
2. anonymous → have no topic, go out of the loop because of the panic.
3. That’s all, the Withdrawal event is not processed while it should have released funds on the receiving chain.

That means that if users interact with a contract that emits some anonymous events, and then interact with either a ZRC20 or a connector in the same transaction, these logs won’t be processed.

That kind of action may happen when using the multi-call contract.

Below a test script written in go that panic when the Topics is empty:

package keeper_test

import (
	"testing"

	"github.com/ethereum/go-ethereum/accounts/abi/bind"
	"github.com/ethereum/go-ethereum/common"
	ethtypes "github.com/ethereum/go-ethereum/core/types"
	zrc20 "github.com/zeta-chain/protocol-contracts/pkg/contracts/zevm/zrc20.sol"
)

func TestEvent(t *testing.T) {
    log := ethtypes.Log {
        Address: common.Address{},
        Topics: []common.Hash{},
        Data: []byte{},
        BlockNumber: 0,
        TxHash: common.Hash{},
        TxIndex: 0,
        BlockHash: common.Hash{},
        Index: 0,
        Removed: false,
    }
    zrc20ZEVM, err := zrc20.NewZRC20Filterer(log.Address, bind.ContractFilterer(nil))
    if err != nil {
        return
    }
    _, err = zrc20ZEVM.ParseWithdrawal(log)
    if err != nil {
        return
    }
}

Tools Used

Manual inspection

Recommended Mitigation Steps

• We recommend upgrading to the prior version of Geth at least (≥1.11.6).
• If for some business logic reason, the Zetachain team cannot upgrade to a newer version of Geth, we recommend implementing a size check on the size of the length ensuring that the length of the log.Topics > 0 before processing it.

References:

• PR that fixed the Topics issue on Geth → accounts/abi/bind: handle UnpackLog with zero topics ethereum/go-ethereum#26920

Assessed type

Invalid Validation

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[c4-pre-sort]

DadeKuma marked the issue as sufficient quality report

[c4-sponsor]

lumtis (sponsor) acknowledged

[0xean]

While the warden clearly shows the POC for the panic, the impact and proof of how this can be crafted into a malicious act is much less clear but within reason to expect that some sort of denial of service would be feasible at a minimum.

[c4-judge]

0xean marked the issue as satisfactory