Off-chain (Bridge) Full Re-Assessment - RuneMine

There is an integer overflow vulnerability in the appendUtxos function of the Bitcoin adapter. This issue arises from the calculation of the transaction fee, which could lead to unexpected behavior, including the creation of transactions with incorrect fees or the inability to create transactions at all.

 int64 total amount of input utxos
 */
func (a *Adapter) appendUtxos(tx *wire.MsgTx, receiver string, runeID string, runeAmount string) ([]int64, error) {
	if runeAmount == "0" {
		return nil, fmt.Errorf("runeAmount must be greater than 0")
	}

	var res struct{ FastestFee int64 }
	err := util.Http(&res, "GET", "https://mempool.space/api/v1/fees/recommended", "", nil)
	if err != nil {
		return nil, err
	}

	// add payment utxos
	fee := res.FastestFee * 150 / 100
	if fee > maxFeePerByte {
		fee = maxFeePerByte
	}
	costSatoshis := int64(runeUtxoValue + runeUtxoValue + (fee * bytesPerRuneUtxo))
	inputSatoshis := int64(0)
	inputAmounts := []int64{}

	utxos, err := a.client.Utxos(a.multisig)
	if err != nil {
		return nil, err
	}
	sort.Slice(utxos, func(i, j int) bool {
		if utxos[i].Status.Confirmed != utxos[i].Status.Confirmed {
			return utxos[i].Status.Confirmed
		}
		return utxos[i].Value > utxos[j].Value
	})

	for _, utxo := range utxos {
		value := int64(utxo.Value)
		txId := utxo.Txid
		voutIdx := uint32(utxo.Vout)

		// don't spend our "dust" runes utxos
		// exist with more than 10k satoshis
		if value <= minimumDustThreshold {
			continue
		}

		// skip recently spent utxo (that the utxo api might not have indexed)
		if isSpent(fmt.Sprintf("%s:%d", txId, voutIdx)) {
			continue
		}

		inputSatoshis += value
		inputAmounts = append(inputAmounts, value)

		hash, err := chainhash.NewHashFromStr(txId)
		if err != nil {
			return nil, err
		}

		tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(hash, voutIdx), nil, nil))
		if inputSatoshis > costSatoshis {
			break
		}
	}

	if inputSatoshis < costSatoshis {
		return nil, fmt.Errorf("not enough utxos to cover cost: %d < %d", inputSatoshis, costSatoshis)
	}

	changeSatoshis := inputSatoshis - costSatoshis

	// add runes utxos
	runesUtxos, err := a.indexer.GetUtxos(a.multisig)
	if err != nil {
		return nil, err
	}
	targetAmount := util.StringToInt(runeAmount)
	currentAmount := int64(0)
	for _, u := range runesUtxos.Utxos {
		idParts := strings.Split(u.Id, ":")
		index := uint32(util.StringToInt(idParts[1]))
		// skip recently spent utxo (that the utxo api might not have indexed)
		if isSpent(fmt.Sprintf("%s:%d", idParts[0], index)) {
			continue
		}

		b, ok := u.Balances[runeID]
		if !ok {
			continue
		}
		amount, err := b.Int64()
		if err != nil {
			continue
		}
		currentAmount += amount

		inputAmounts = append(inputAmounts, int64(u.Value))
		hash, err := chainhash.NewHashFromStr(idParts[0])
		if err != nil {
			return nil, err
		}

		tx.AddTxIn(wire.NewTxIn(wire.NewOutPoint(hash, index), nil, nil))
		if currentAmount >= targetAmount {
			break
		}
	}

	if currentAmount < targetAmount {
		return nil, fmt.Errorf("not enough utxos (RUNES) to cover cost: %d < %d", currentAmount, targetAmount)
	}

	// add outputs - it should be extracted into a separate function

	for _, out := range []struct {
		Address string
		Amount  int64
	}{{a.multisig, runeUtxoValue}, {receiver, runeUtxoValue}, {a.multisig, changeSatoshis}} {
		outputAddress, err := btcutil.DecodeAddress(out.Address, a.net)
		if err != nil {
			return nil, err
		}
		pkScript, err := txscript.PayToAddrScript(outputAddress)
		if err != nil {
			return nil, err
		}
		tx.AddTxOut(wire.NewTxOut(out.Amount, pkScript))
	}

	split := strings.Split(runeID, ".")

	runestoneInts := []*big.Int{big.NewInt(0), util.StringToBi(split[0]), util.StringToBi(split[1]), util.StringToBi(runeAmount), big.NewInt(1)}
	runestoneBytes := []byte{}
	zero := big.NewInt(0)
	for _, i := range runestoneInts {
		for (&big.Int{}).Rsh(i, 7).Cmp(zero) > 0 {
			b := i.Bytes()
			runestoneBytes = append(runestoneBytes, b[len(b)-1]|0b1000_0000)
			i.Rsh(i, 7)
		}
		b := i.Bytes()
		if len(b) > 0 {
			runestoneBytes = append(runestoneBytes, b[len(b)-1])
		} else {
			runestoneBytes = append(runestoneBytes, 0)
		}
	}
	runestoneBytes = append([]byte{106, 93, byte(len(runestoneBytes))}, runestoneBytes...)
	tx.AddTxOut(&wire.TxOut{PkScript: runestoneBytes})

	return inputAmounts, nil
}

Use big.Int for fee calculations to avoid overflow issues.

SOLVED: The RuneMine team solved the issue by implementing the suggested recommendations.

The current implementation does not properly handle decimal places in amount calculations, potentially leading to incorrect fee calculations and transfer amounts.

In the Bridge function of the Solana adapter, the amount calculation does not account for the decimal places of the token. This can result in incorrect fee calculations and transfer amounts, especially for tokens with different decimal places.

fee := uint64(util.StringToInt(transfer.Amount)) * a.feePercent / 1_000
amountWithoutFee := uint64(util.StringToInt(transfer.Amount)) - fee

Modify the amount calculation to account for these decimal places.

SOLVED: The RuneMine team solved the issue by using created by them and their decimals match the runes ones.

The current implementation of the Solana poller uses panic for error handling in several places. This approach can cause the entire bridge process to halt when a single error occurs.

Problematic Code Example

1. In the poll function:


util.Check(err) // This function panics on error

2. Direct use of panic:


if fee < 50000000 {
panic(fmt.Sprintf("sol fee paid to small to cover transaction cost: %d", fee))
}


3. In utility functions:


func Check(err error) {
if err != nil {
panic(err)
}
}

There's no visible panic recovery mechanism in the main function to catch panics from goroutines. This could lead to goroutine leaks and partial system failure.

RISK ACCEPTED: The RuneMine team accepted the risk of the issue.

The current HTTP utility functions (Http and HttpVal) do not implement any timeout mechanism. This could lead to hanging requests if the server doesn't respond.

Current behavior:
- HTTP requests are made using the default HTTP client without any timeout setting.
- Long-running or non-responsive requests could block indefinitely.

1. Implement a configurable timeout for HTTP requests.
2. Use http.Client with a timeout instead of http.DefaultClient.
3. Add context support for cancellation and timeouts.

Reference : https://pkg.go.dev/net/http

SOLVED: The RuneMine team solved the issue by implementing the suggested recommendations.

The bridge application uses a time-based seeding method for its random number generator:

rand.Seed(uint64(time.Now().UnixNano()))

Implement a cryptographically secure random number generator. Consider using `crypto/rand` from the Go standard library:

seed, _ := crypto/rand.Int(crypto/rand.Reader, big.NewInt(1<<63-1))
rand.Seed(seed.Uint64())

SOLVED: The RuneMine team solved the issue by implementing the suggested recommendations.

The current implementation stores sensitive private keys as environment variables:

BitcoinPrivateKeys: util.EnvArray("BITCOIN_PRIVATE_KEYS"),
SolanaPrivateKeys:  util.EnvArray("SOLANA_PRIVATE_KEYS"),

This approach poses security risks, as environment variables can be exposed through system logs, debugging output, or compromised system access.

Implement a secure key management system:

1. Use a dedicated key management service (e.g., HashiCorp Vault, AWS KMS, or Azure Key Vault).

2. Encrypt keys at rest and in transit.

3. Implement proper access controls and auditing for key operations.

4. Use temporary, rotating credentials instead of long-lived keys where possible.

RISK ACCEPTED: The RuneMine team accepted the risk of the issue.

The current server configuration uses HTTP instead of HTTPS, as evidenced by the following line of code:

http.ListenAndServe(":"+port, nil)

Implement HTTPS by using TLS/SSL. This involves the following steps:

1. Obtain an SSL/TLS certificate from a trusted Certificate Authority (CA).
2. Configure the server to use HTTPS instead of HTTP.
3. Implement proper TLS settings to ensure strong encryption and security.

RISK ACCEPTED: The RuneMine team accepted the risk of the issue.

The current implementation of Bitcoin transactions in the bridge doesn't set the nLockTime field. This could potentially expose our transactions to fee sniping attacks during blockchain reorganizations.

Consider using nLockTime field.

SOLVED: The RuneMine team solved the issue by implementing the suggested recommendations.

The current configuration uses single RPC endpoints for both Bitcoin and Solana networks without any backup options. This creates a single point of failure that could lead to service disruptions.

1. Implement RPC redundancy:

- Add multiple RPC endpoints for both Bitcoin and Solana networks

- Implement a failover mechanism to switch between RPCs

- Consider using a load balancing approach to distribute requests across multiple RPCs

2. Enhance configuration management:

- Move RPC URLs to a configuration file or environment variables

- Allow easy addition and removal of RPC endpoints without code changes

3. Implement health checks and monitoring:

- Regularly check the health of all RPC endpoints

- Automatically switch to healthy RPCs when issues are detected

- Alert the operations team when RPC issues are encountered

RISK ACCEPTED: The RuneMine team accepted the risk of the issue.

Multiple outdated dependencies were found in the codebase. These dependencies include versions with known security vulnerabilities that pose significant risks to the application’s security and stability.

+-----------------------------------------------------+----------+-------------+--------+------------------+
|                       MODULE                        | VERSION  | NEW VERSION | DIRECT | VALID TIMESTAMPS |
+-----------------------------------------------------+----------+-------------+--------+------------------+
| github.com/aws/aws-sdk-go-v2                        | v1.27.2  | v1.30.0     | true   | true             |
| github.com/aws/aws-sdk-go-v2/config                 | v1.27.18 | v1.27.22    | true   | true             |
| github.com/aws/aws-sdk-go-v2/service/secretsmanager | v1.30.0  | v1.32.0     | true   | true             |
| github.com/btcsuite/btcd                            | v0.24.0  | v0.24.2     | true   | true             |
| github.com/gagliardetto/solana-go                   | v1.10.0  | v1.11.0     | true   | true             |
| github.com/jackc/pgx/v5                             | v5.5.5   | v5.6.0      | true   | true             |
| gorm.io/driver/postgres                             | v1.5.7   | v1.5.9      | true   | true             |
| gorm.io/gorm                                        | v1.25.9  | v1.25.10    | true   | true             |
+-----------------------------------------------------+----------+-------------+--------+------------------+

Update all affected dependencies to the latest possible version.

SOLVED: The RuneMine team solved the issue by upgrading dependencies.

Bitcoin block validation ensures that every block added to the blockchain is legitimate and maintains the integrity of the network. It involves verifying the block header, ensuring proof of work, validating all transactions, checking the block reward, and ensuring the block integrates correctly into the blockchain. This process is fundamental to the security and functionality of Bitcoin.

Sometimes, temporary forks or competing chains can occur. A fork happens when two miners find a block at the same time, leading to two versions of the blockchain. The network will eventually adopt the longest chain (with the most proof of work), potentially abandoning blocks from shorter chains.

Time based validations cannot be considered safe as they are not validating the number of blocks confirmed.

func (p *Poller) poll(ctx context.Context, transfers chan<- *bridge.Transfer, errors chan<- error) {
	response, err := p.indexer.GetTransfersByDestination(p.bridgeMultisig)
	if err != nil {
		errors <- err
		return
	}
	for _, t := range response.Transfers {
		tx, err := p.indexer.GetTransaction(t.Tx)
		if err != nil {
			errors <- err
			continue
		}

		// to handle reorgs, wait for tx to be 20m past 1st confirmation
		if time.Now().Unix()-t.Time < 20*60 {
			continue
		}

Monitor the number of confirmations of a transaction to avoid potentials reorganizations, so that triggering operations like minting or burn before the transactions is fully ensured only afterwards.

SOLVED: The RuneMine team solved the issue by increasing confirmation numbers, and they are going to monitor it.

The current bridge function in the Bridge struct lacks a minimum transfer amount check. While this check is implemented in the BTC and Solana pollers, adding it to the central bridge function would provide an additional layer of security and consistency.

package bridge

import (
	"context"
	"fmt"
	"log"
	"runemine-bridge/util"
	"slices"
	"strconv"
	"time"

	"gorm.io/gorm"
)

type Bridge struct {
	repository     *TransferRepository
	runesWhitelist []string
	Pollers        map[int]Poller
	Adapters       map[int]Adapter
}

func NewBridge(config *Config, db *gorm.DB) *Bridge {
	return &Bridge{
		repository:     NewTransferRepository(db),
		runesWhitelist: config.RunesWhitelist,
		Pollers:        map[int]Poller{},
		Adapters:       map[int]Adapter{},
	}
}

func (b *Bridge) SetPoller(network int, p Poller) {
	b.Pollers[network] = p
}

func (b *Bridge) SetAdapter(network int, p Adapter) {
	b.Adapters[network] = p
}

func (b *Bridge) Run(ctx context.Context) chan error {
	errorChannel := make(chan error)
	for network, p := range b.Pollers {
		go b.run(ctx, network, p, errorChannel)
	}
	return errorChannel
}

func (b *Bridge) run(ctx context.Context, network int, p Poller, errorChannel chan<- error) {
	transferChannel := make(chan *Transfer, 10)
	defer close(transferChannel)
	go p.Poll(ctx, transferChannel, errorChannel)
	for {
		select {
		case transfer := <-transferChannel:
			b.bridge(ctx, network, transfer)
		case <-ctx.Done():
			return
		}
	}
}

func (b *Bridge) bridge(ctx context.Context, network int, transfer *Transfer) {
	defer func() {
		if err := recover(); err != nil {
			transfer.Updated = time.Now()
			transfer.Status = "error"
			transfer.Error = fmt.Sprintf("panic: %v", err)
			log.Println("bridge panic", transfer.ID, transfer.Error)
			util.Check(b.repository.Save(transfer))
		}
	}()

	_, err := b.repository.FindById(transfer.ID)
	if err == nil {
		//log.Println("existing", transfer.ID)
		return
	}

	log.Println("bridging", transfer.ID)
	transfer.Source = network
	transfer.Created = time.Now()
	transfer.Updated = transfer.Created
	if transfer.Status == "error" {
		log.Println("bridge error", transfer.ID, transfer.Error)
		util.Check(b.repository.Save(transfer))
		return
	}

	if !slices.Contains(b.runesWhitelist, transfer.Rune) {
		transfer.Status = "error"
		transfer.Error = "rune not whitelisted"
		log.Println("bridge error", transfer.ID, transfer.Error)
		util.Check(b.repository.Save(transfer))
		return
	}

	if transfer.To == "" {
		transfer.Status = "error"
		transfer.Error = "missing to wallet"
		log.Println("bridge error", transfer.ID, transfer.Error)
		util.Check(b.repository.Save(transfer))
		return
	}

	if transfer.Amount == "" || transfer.Amount == "0" {
		transfer.Status = "error"
		transfer.Error = "missing amount"
		log.Println("bridge error", transfer.ID, transfer.Error)
		util.Check(b.repository.Save(transfer))
		return
	}

	a, ok := b.Adapters[transfer.Target]
	if !ok {
		transfer.Status = "error"
		transfer.Error = "unsupported network " + strconv.Itoa(transfer.Target)
		log.Println("bridge error", transfer.ID, transfer.Error)
		util.Check(b.repository.Save(transfer))
		return
	}

	// Save now to avoid calling adapter.Bridge twice
	transfer.Status = "sending"
	util.Check(b.repository.Save(transfer))

	targetId, err := a.Bridge(ctx, transfer)
	transfer.Status = "sent"
	transfer.Updated = time.Now()
	transfer.TargetID = targetId
	if err != nil {
		transfer.Status = "error"
		transfer.Error = err.Error()
		log.Println("bridge error", transfer.ID, transfer.Error)
	}
	util.Check(b.repository.Save(transfer))
	log.Println("bridged", transfer.ID, transfer.TargetID)
}

Implement a minimum transfer amount check in the bridge function:

1. Add a minimumTransferAmount field to the Bridge struct or Config.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

The current implementation uses a mix of logging methods, including fmt.Println, log.Println, and direct panic calls. This inconsistency makes it difficult to manage log levels and format logs uniformly across the bridge system.

Current issues:
1. Inconsistent use of logging methods (fmt.Println, log.Println, panic).
2. Lack of log levels (DEBUG, INFO, ERROR, etc.).
3. No centralized control over log output and formatting.

Implement a standardized logging system across the bridge, with the following features:
1. Define log levels (DEBUG, INFO, WARN, ERROR, FATAL).
2. Create a centralized logging package or utility.
3. Replace all current logging calls with the new standardized logger.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

The Bridge function in the provided code uses time.Now() to set the Created and Updated fields of transactions. This approach can lead to inconsistencies in transaction timestamps across different nodes or systems, especially in a distributed environment.

Consider using Network Time Protocol (NTP) to ensure all nodes are synchronized to a common time source.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

The current seed generation process in the generateKey function potentially uses a weak source of entropy, which could compromise the security of the generated keys.

func generateKey(i int) (string, []byte) {
    if mnemonic := env("SEED"+strconv.Itoa(i), ""); mnemonic != "" {
        seed, err := bip39.MnemonicToSeed(mnemonic, "")
        check(err)
        return mnemonic, seed
    }
    entropy, err := bip39.GenerateEntropy(bip39.EntWords12)
    check(err)
    mnemonic, seed, err := bip39.Mnemonic(entropy, "")
    check(err)
    return mnemonic, seed
}

1. Use a cryptographically secure random number generator (CSPRNG) explicitly, such as crypto/rand from the Go standard library.
2. Avoid using environment variables for seed storage. If needed, use a secure key management system.
3. Increase the entropy to 256 bits (24 words) for enhanced security.
4. Add a strong passphrase to the seed generation process.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

In the `Bridge` function, the fee calculation uses the following formula:

fee := uint64(util.StringToInt(transfer.Amount)) * a.feePercent / 1_000

For very small transfer amounts, this could result in a fee of 0 due to integer division. This may allow free transactions, which could be exploited and may not be economically viable for the system.

1. Implement a minimum fee threshold.

2. Use fixed-point arithmetic for more precise fee calculations.

3. Round up fees to the nearest whole number, ensuring a minimum of 1.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

The server implements an overly permissive CORS policy:

w.Header().Set("Access-Control-Allow-Origin", "*")
w.Header().Set("Access-Control-Allow-Methods", "HEAD, GET, POST")
w.Header().Set("Access-Control-Allow-Headers", "Accept, Content-Type, Authorization")

1. Restrict the `Access-Control-Allow-Origin` header to specific, trusted domains.

2. Implement a whitelist of allowed origins.

3. Consider using environment variables to configure allowed origins for different environments.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.

When sending a transaction with a different Bitcoin Opcode, the bridge processed the transaction and did not verify that the OP_RETURN opcode was used. This eventually resulted in the user's wallet being locked. Subsequent transactions failed along with the error bad-txns-inputs-missingorsend.

The Bitcoin Opcode was changed from 106 (OP_RETURN) to 97 (NOP).

The bridge could ensure that the correct Bitcoin Opcode is used in transactions, namely 106 - OP_RETURN.

ACKNOWLEDGED: The RuneMine team acknowledged the issue.