Consortium - Lombard

The suggestNewPendingWork function is responsible for proposing new work for the consensus process by fetching pending proposals from the database, unmarshalling them, and then proposing them for further processing. However, the current implementation lacks robust error handling, leading to a critical vulnerability. Specifically, if an attacker were to introduce a malicious proposal into the system—one that causes an error during the unmarshalling process (req.Unmarshal(p.Message)) or during the proposal submission (s.Propose(req)), the function would trigger a log.Panicf. This panic would cause the service to crash, halting the consensus process across the nodes.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L230-L250

func (s *Service) suggestNewPendingWork() {
	_ = s.WaitForLeader(context.TODO())
	if s.proposal != nil {
		return
	}
	proposals := s.databaseService.GetProposalsByStatusOrderByIndex(types.ProposalStatus_PROPOSAL_STATUS_QUEUED, 0, 1)
	if len(proposals) == 0 {
		return
	}
	p := proposals[0]
	req := &types.RequestProposal{}
	if err := req.Unmarshal(p.Message); err != nil {
		log.Panicf("failed to parse proposal message: %+v", err)
	}
	// make this job forcibly pending to let other workers know that we're working on this job
	req.Queued = true
	err := s.Propose(req)
	if err != nil {
		log.Panicf("failed to propose message: %+v", err)
	}
}

Such a crash could be exploited to disrupt the consensus mechanism, leading to a denial-of-service (DoS) attack on the system.

To mitigate this vulnerability, replace the log.Panicf calls with proper error handling mechanisms that safely log the error and allow the service to continue operating. Specifically, instead of panicking, the function should log the error, skip the malicious proposal, and continue processing other proposals. Additionally, consider implementing validation and sanitization checks on incoming proposals to detect and reject malicious or malformed proposals before they reach critical points in the code.

Remediation Plan

SOLVED: The Lombard team solved this issue by following the aforementioned recommendation and removing panics.

The handleJobReply function in the Service struct processes reply messages in the consensus mechanism by invoking the appropriate handler for the proposal type. If an error occurs in the HandleReply function of the handler, the current context is cleared by calling h.Clear(). While this might seem like a logical way to reset the state, it introduces a vulnerability: an attacker could craft a malicious message that triggers an error in HandleReply, causing the node to clear its current task context prematurely. This could disrupt the consensus process, as the node would lose its ability to process further legitimate replies, effectively stalling its participation in the ongoing consensus.

https://github.com/lombard-finance/consortium-node/blob/58ac32035abc505a66991a8c69df99e9e20ba202/shared/consensus/service.go#L518-L533

func (s *Service) handleJobReply(proposal *types.Proposal, reply *types.RequestProposal) error {
	h := s.controllers[proposal.Type]
	if h == nil {
		return fmt.Errorf("there is no handler for proposal type (%s)", proposal.Type.String())
	}
	s.logger.Infof("handle reply (%s)", proposal.Id)
	result, err := h.HandleReply(reply)
	if err != nil {
		_ = h.Clear()
		return err
	}
	if result != nil {
		return s.propose(result)
	}
	return nil
}

For example, in the case of the signEarlyUnbondController's HandleReply function, several errors could be triggered by malicious messages, such as invalid signatures or malformed transaction data. This would result in the context being cleared, leaving the node unable to continue processing the consensus task.

https://github.com/lombard-finance/consortium-node/blob/ebab6b7bea92d1e94768e307c5a3c001ba9367d4/shared/consensus/internal/controller/sign_early_unbond.go#L183-L194

func (c *signEarlyUnbondController) HandleReply(reply *types.RequestProposal) (*types.RequestProposal, error) {
	if c.signingData == nil {
		log.Tracef("player (%s) state is clean", c.consensusFeed.HostID())
		return nil, nil
	}

	reply2 := reply.GetSignEarlyUnbondReply()
	if reply2 == nil {
		return nil, errors.Errorf("unable to handle incompatible reply message")
	} else if err := validation.Validate(reply2); err != nil {
		return nil, err
	}
	
// ...

There are two possible approaches to address the issue:

1. Identify recoverable and non-recoverable errors: Evaluate which errors represent a non-recoverable state in the consensus, requiring the process to be restarted, and which do not pose a significant risk, allowing the current task to continue processing. This approach involves distinguishing between errors that should clear the consensus state and those that should not, though it may be less scalable and effective.

2. Reject external messages and penalize malicious behavior: Identify and validate consensus participants, rejecting messages from external agents. Additionally, implement a penalty mechanism to report and sanction malicious behavior among nodes. While more complex, this approach is more effective and decentralized for managing issues and misbehavior within the consensus process.

Remediation Plan

RISK ACCEPTED: The Lombard team accepted the risk of this issue and states:

All errors in the application-specific handler are treated as non-recoverable. Errors that are typically considered recoverable, such as those caused by validators sending spam or malformed requests, are not a concern in this context. This is because the consortium operates within a permissioned network where only vetted validators are allowed. Additionally, all requests are routed through the Lombard backend API, which enforces rigorous validation. As a result, such erroneous requests are not expected to occur.

When a new request is received through any of the available gateways, it is processed and verified by passing several checks over the requests depending on its type. After all validations and check are passed successfully, this request is proposed to the consensus in order to handle it by all the involved players in the threshold key. This proposal is made by an initial node which will be executing the ProposeAndWait function.

This function is in charge of generating a new random message ID for the proposal and execute the DragonBoat API to add a new proposal to the process. Since this is an asynchronous process, the proposer has to wait until the proposal has been enqueued in database by checking every 100 milliseconds this proposal in the database, in case this proposal hasn’t been included in the database in less than 5 minutes, this functions will return an error.

Since this function is open to be executed through any gateway and it blocks its thread’s execution until the request is enqueued or after 5 minutes, its implementation is highly inefficient and even could lead to a denial of service if the node is flooded with multiple proposal requests.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L381-L407

func (s *Service) ProposeAndWait(ctx context.Context, message *types.RequestProposal) (*types.Proposal, error) {
	ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Minute)
	defer cancel()
	if message.Id == "" {
		message.Id = uuid.New().String()
	}
	err := s.propose(message)
	if err != nil {
		return nil, err
	}
	id, _ := uuid.Parse(message.Id)
	timer := time.Tick(100 * time.Millisecond)
	for {
		select {
		case <-timer:
		case <-ctx.Done():
			return nil, ctx.Err()
		}
		p := s.databaseService.GetProposalOrNil(id)
		if p == nil {
			continue
		}
		if p.Status != types.ProposalStatus_PROPOSAL_STATUS_QUEUED {
			return p, nil
		}
	}
}

It is recommended to rewrite the logic of this function to keep it asynchronous or to adjust the timeout to prevent from denial of services attacks.

Remediation Plan

PENDING: The Lombard team will fix this issue in future releases and states:

The consortium API supports both asynchronous and synchronous workflows. The asynchronous flow is the preferred method and is currently implemented across several API handlers, with plans to extend its use to all handlers in future updates. The synchronous flow, which utilizes the ProposeAndWait method, is reserved exclusively for API calls with deterministic logic. In cases where a request has already been processed, the synchronous approach returns the result of the previous execution to maintain consistency.
While the synchronous ProposeAndWait approach remains in place for backward compatibility, it is now considered deprecated and will be phased out in future releases. Transitioning away from this method is necessary to standardize on the more efficient and scalable asynchronous workflow.

In the current implementation of a Threshold Signature Scheme (TSS) that communicates through a consensus mechanism (Dragonboat), there is a notable absence of slashing or penalty mechanisms for nodes that act maliciously within the network. This gap is particularly concerning given the vulnerabilities identified in previous issues, such as "Malicious Messages Can Clear Consensus State". In that issue, it was highlighted that malicious nodes could exploit the error handling in consensus processes to force nodes to clear their current consensus state, effectively causing a denial-of-service (DoS) attack on the network.

The TSS relies on a subset of nodes (signers/players) to collaboratively generate signatures on transactions or other data, making it critical that all participating nodes adhere strictly to the protocol. However, without a mechanism to penalize nodes for behaviors that deviate from the expected protocol (such as sending incorrect or malicious data that leads to consensus failure), there is little deterrent against such actions. This lack of penalties could lead to scenarios where malicious or faulty nodes disrupt the TSS process, causing delays, incorrect signing, or complete failure of critical operations.

It is recommended to implement a penalty and slashing mechanism based on collective node reporting in a decentralized manner. Nodes that receive erroneous or incorrect messages from peers participating in the consensus process should have the capability to report such behaviors to the broader network. If a consensus majority confirms the reported misbehavior, the mechanism should then enact penalties such as temporary suspension or permanent expulsion of the offending node from the consensus process. This system of checks and balances will ensure that all participating nodes adhere strictly to protocol rules, thereby enhancing the integrity and reliability of the entire network. Bear in mind that, the implementation of this mechanism would require careful design to ensure that it cannot be misused for competitive or malicious purposes against honest nodes.

Remediation Plan

RISK ACCEPTED: The Lombard team accepted the risk of this issue with the following statement:

While we currently do not have a technical slashing mechanism in place, our network operates within a permissioned environment with carefully vetted validators. Any malicious activity would be addressed through the enforcement of our contractual agreements, ensuring the immediate removal of any offending validators. This contractual framework provides a strong layer of security, maintaining the integrity and trustworthiness of the consortium.

The functions suggestNewPendingWork and handleJobRequest within the Service class are integral to handling proposals within a consensus-based system. Both functions operate concurrently and access shared resources, such as s.proposal, which introduces a high risk of race conditions. These race conditions can occur because both functions check, modify, or set the state of s.proposal without any form of synchronization or locking mechanism In suggestNewPendingWork, if a proposal is not currently set, the function fetches a queued proposal, unmarshals it, and proposes it to the network.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L230-L250

func (s *Service) suggestNewPendingWork() {
	_ = s.WaitForLeader(context.TODO())
	if s.proposal != nil {
		return
	}
	proposals := s.databaseService.GetProposalsByStatusOrderByIndex(types.ProposalStatus_PROPOSAL_STATUS_QUEUED, 0, 1)
	if len(proposals) == 0 {
		return
	}
	p := proposals[0]
	req := &types.RequestProposal{}
	if err := req.Unmarshal(p.Message); err != nil {
		log.Panicf("failed to parse proposal message: %+v", err)
	}
	// make this job forcibly pending to let other workers know that we're working on this job
	req.Queued = true
	err := s.Propose(req)
	if err != nil {
		log.Panicf("failed to propose message: %+v", err)
	}
}

However, during this process, handleJobRequest might simultaneously check or modify s.proposal based on another incoming request. This lack of coordinated access control can lead to inconsistent system states or errors, such as duplicate proposal processing or incorrect proposal handling.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L466-L501

func (s *Service) handleJobRequest(proposal *types.Proposal, req *types.RequestProposal) error {
	// don't execute same job twice
	if s.proposal != nil {
		if s.proposal.Id == proposal.Id {
			return nil
		}
		log.Warnf("there is already pending job (%s), next one (%s) can't be proceed", s.proposal.Id, proposal.Id)
		return nil
	}
	// check proposal status
	{
		p := s.databaseService.GetProposalOrNil(uuid.MustParse(proposal.Id))
		if p != nil && p.Status != types.ProposalStatus_PROPOSAL_STATUS_QUEUED {
			log.Warnf("proposal (%s) is already finished (%s), skipping request", p.Id, p.Status)
			return nil
		}
	}
	if err := s.databaseService.SaveProposal(proposal); err != nil {
		return err
	}
	h := s.controllers[proposal.Type]
	if h == nil {
		return fmt.Errorf("there is no handler for proposal type (%s)", proposal.Type.String())
	}
	result, err := h.HandleRequest(req)
	if err != nil {
		_ = h.Clear()
		return err
	}
	s.proposal = proposal
	log.Infof("handled proposal (%s) start", proposal.Id)
	if result != nil {
		return s.propose(result)
	}
	return nil
}

Furthermore, the error handling in suggestNewPendingWork uses log.Panicf which can crash the service when an error occurs during proposal unmarshalling or submission, exacerbating the impact of these race conditions.

To mitigate these risks and ensure robust operation, implementing a proper synchronization mechanism is essential. Use mutexes or other concurrency control structures to manage access to shared resources, such as s.proposal.

Remediation Plan

SOLVED: The Lombard team solved this issue by implementing locks/mutexes.

The functions HandleReply in both the signEarlyUnbondController and signTimelockWithdrawController handle incoming reply messages related to the early unbonding and timelock withdrawal processes, respectively. In these functions, there are several scenarios where the createReply method is called to generate a new reply message, even when an error or an incompatible situation occurs. This approach can lead to an issue where the consensus mechanism might enter a recursive loop, repeatedly sending reply messages. This is problematic because these functions are intended to handle a reply, not generate a new one, and returning a new reply could result in potential race conditions, and incorrect consensus behavior.

https://github.com/lombard-finance/consortium-node/blob/ebab6b7bea92d1e94768e307c5a3c001ba9367d4/shared/consensus/internal/controller/sign_early_unbond.go#L214-L233

	mfa, err2 := c.cubistService.GetMfa(c.earlyUnbondingParams.MfaId)
	if err2 != nil {
		// NOTE: could be race when mfa already aproved by another node
		if strings.Contains(err2.Error(), "not found") {
			return c.createReply(reply, nil, nil), nil
		}

		return nil, errors.Wrapf(err2, "get mfa %s", c.earlyUnbondingParams.GetMfaId())
	}

	// if mfa approved just wait for signature in reply, because if we go to approve it in this node we can get race condition
	if len(mfa.GetStatus().GetApprovedBy()) < int(mfa.GetStatus().GetCount()) {
		if !cubistcommon.IsAllowedToApprove(mfa, c.signingData.userId) {
			return nil, errors.Errorf("user (%s) is not allowed to approve transactions", c.signingData.userId)
		}

		// NOTE: it's can't happen, because if tx already processed we found it at beginning
		if cubistcommon.IsApprovedByUser(mfa, c.signingData.userId) {
			return c.createReply(reply, nil, nil), nil
		}

https://github.com/lombard-finance/consortium-node/blob/ebab6b7bea92d1e94768e307c5a3c001ba9367d4/shared/consensus/internal/controller/sign_early_unbond.go#L216-L219

// NOTE: it's can't happen, because if tx already processed we found it at beginning
if cubistcommon.IsApprovedByUser(mfa, c.signingData.userId) {
	return c.createReply(reply, rawTx), nil
}

https://github.com/lombard-finance/consortium-node/blob/f083a2d1fcc7b29b75db178f0d58ca09dd163297/shared/consensus/internal/controller/sign_timelock_withdraw.go#L234-L237

// NOTE: it's can't happen, because if tx already processed we found it at beginning
if cubistcommon.IsApprovedByUser(mfa, c.signingData.userId) {
	return c.createReply(reply, rawTx), nil
}

Modify the HandleReply functions to return nil, nil instead of calling createReply in situations where an error occurs or when the conditions are not met for processing the reply message.

Remediation Plan

SOLVED: The Lombard team solved this issue by returning nil, nil as it was recommended.

During the generateDepositAddressController class execution, there are several checks being executed in order to guarantee that the request is safe to be processed. One of those checks is run in the verifyDestinationSignature function, in charge to validate several parameters related to the input signature.

https://github.com/lombard-finance/consortium-node/blob/93caa5febffada61941b2891607f3ba5389c39a5/shared/consensus/internal/controller/generatedepositaddress.go#L101-L108

chainId, ok := c.blockchainService.ChainIds[req2.GetToChain()]
if !ok {
	err = errors.Errorf("unsupported chain (%s)", chainId)
}

if err = verifyDestinationSignature(req2, chainId); err != nil {
	return
}

Apart from checking the chain’s destination following the specified chainId, it also tries to retrieve the public key associated to the signature used in the request by using the blockchain.VerifyMessage() function.

https://github.com/lombard-finance/consortium-node/blob/93caa5febffada61941b2891607f3ba5389c39a5/shared/consensus/internal/controller/generatedepositaddress.go#L253-L262

func verifyDestinationSignature(req *types.GenerateDepositAddressRequest, chainID *big.Int) error {
	switch req.GetToChain() {
	case types.Blockchain_BLOCKCHAIN_ETHEREUM, types.Blockchain_BLOCKCHAIN_MANTA, types.Blockchain_BLOCKCHAIN_MANTLE, types.Blockchain_BLOCKCHAIN_XLAYER, types.Blockchain_BLOCKCHAIN_SCROLL:
		// verify eth
		thresholdKey := uuid.MustParse(req.GetThresholdKey())

		address, err := blockchain.VerifyMessage(getTemplatedMessage(chainID, thresholdKey), req.GetToAddressSignature())
		if err != nil {
			return errors.Wrap(err, "verify message")
		}

The VerifyMessage function check some parameters of the signed message such us the length and EIP-191 format. After all those checks are passed, the node tries to retrieve the public key from the signature by executing the crypto.SigToPub function.

https://github.com/lombard-finance/consortium-node/blob/892060a7055ddf41d8c4628b7d236ca7b62c2bec/shared/blockchain/web3.go#L250-L274

func VerifyMessage(message string, signedMessage []byte) ([]byte, error) {
	if len(signedMessage) < 64 {
		return nil, errors.Errorf("wrong signature length (%d < 64)", len(signedMessage))
	}

	// Hash the unsigned message using EIP-191
	hashedMessage := []byte("\\x19Ethereum Signed Message:\\n" + strconv.Itoa(len(message)) + message)
	hash := crypto.Keccak256Hash(hashedMessage)

	// Handles cases where EIP-115 is not implemented (most wallets don't implement it)
	if signedMessage[64] == 27 || signedMessage[64] == 28 {
		signedMessage[64] -= 27
	}

	// Recover a public key from the signed message
	sigPublicKeyECDSA, err := crypto.SigToPub(hash.Bytes(), signedMessage)
	if sigPublicKeyECDSA == nil {
		err = errors.New("Could not get a public key from the message signature")
	}
	if err != nil {
		return nil, err
	}

	return crypto.PubkeyToAddress(*sigPublicKeyECDSA).Bytes(), nil
}

The crypto.SigToPub function relies on Ecrecover, which is well-known for being vulnerable to malleable signatures. This issue could lead to the re-use of signatures in the protocol.

https://github.com/ethereum/go-ethereum/blob/8adce57b411267faac6cf744aee84f5c51445bb9/crypto/signature_cgo.go#L38

func SigToPub(hash, sig []byte) (*ecdsa.PublicKey, error) {
	s, err := Ecrecover(hash, sig)
	if err != nil {
		return nil, err
	}
	return UnmarshalPubkey(s)
}

It is recommended to use the VerifySignature function prior to retrieving the public key in order to verify if the signature follows the minimal standards, and it’s not a malleable signature.

Remediation Plan

SOLVED: The Lombard team solved this issue by following the aforementioned recommendation.

When a threshold key is proposed to be created, it is necessary to specify a role in its request in order to indicate what the key’s purpose is. This role is checked against a list of available roles inside the protocol in the generateThresholdKeyController.preHandleRequest() method by using the isValidThresholdKeyRole() function.

https://github.com/lombard-finance/consortium-node/blob/98520ab7f43fb2e7f14192149d3092b9d5aa2109/shared/consensus/internal/controller/generatethresholdkey.go#L77-L93

func (c *generateThresholdKeyController) preHandleRequest(req *types.RequestProposal) (result *types.RequestProposal, tk *types.ThresholdKey, err error) {
	// get request and validate it
	req2 := req.GetGenerateThresholdKeyRequest()
	if req2 == nil {
		err = fmt.Errorf("failed to handle incompatible message format")
		return
	} else if err = validation.Validate(req2); err != nil {
		return
	}

	// validate the roles
	for _, role := range req2.Roles {
		if !isValidThresholdKeyRole(role) {
			err = fmt.Errorf("invalid role: %v", role)
			return
		}
	}

As it can be seen below, the available roles follow different purposes inside the node as it’s possible, following the logic of the node, generate multiple threshold keys depending on the action to execute by the node.

https://github.com/lombard-finance/consortium-node/blob/98520ab7f43fb2e7f14192149d3092b9d5aa2109/shared/consensus/internal/controller/generatethresholdkey.go#L33-L46

func isValidThresholdKeyRole(role types.ThresholdKeyRole) bool {
	switch role {
	case types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_NOTARIZE,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_SIGN,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_TRANSACT,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_ISSUE,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_AUTHORITY,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_CA_CERT,
		types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_PLAYER_CERT:
		return true
	default:
		return false
	}
}

By the other hand, these roles should be checked once an action is going to be executed in the node. For example, when the CA generation process is about to start, the node should verify that the threshold key specified in the request has the types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_CA_CERT to be elegible to participate in the process. This also happens when a CSR issue process starts.

https://github.com/lombard-finance/consortium-node/blob/d4f7cc3d283f560abad43560cb02bfd51b4a668c/shared/consensus/internal/controller/generatecacertificate.go#L67-L76

tk = c.databaseService.GetThresholdKeyOrNil(uuid.MustParse(req2.ThresholdKey))
if tk == nil {
	err = fmt.Errorf("no threshold key found with id (%s)", req2.ThresholdKey)
	return
}
requiredRole := types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_CA_CERT
if !internal.HasRequiredRole(tk, requiredRole) {
	err = fmt.Errorf("the threshold key does not have the required role: %s", requiredRole)
	return
}

https://github.com/lombard-finance/consortium-node/blob/e373706ef06f91780179dafaed1d8822bc1bf40f/shared/consensus/internal/controller/issuecertificate.go#L77-L86

tk = c.databaseService.GetThresholdKeyOrNil(uuid.MustParse(req2.ThresholdKey))
if tk == nil {
	err = fmt.Errorf("no threshold key found with id (%s)", req2.ThresholdKey)
	return
}
requiredRole := types.ThresholdKeyRole_THRESHOLD_KEY_ROLE_PLAYER_CERT
if !internal.HasRequiredRole(tk, requiredRole) {
	err = fmt.Errorf("the threshold key does not have the required role: %s", requiredRole)
	return
}

However, in classes such as notarizeTransactionController, notarizeTransactionOutputController and generateDepositAddressController, this condition is not verifying and any threshold signature can be used in their processes no matter what their roles or purposes are.

It is recommended to check the threshold key role in each to request in order to guarantee the correct threshold key it’s being used in each case.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

In the node, the tss-lib from BNB Chain is used to sign multiple actions to execute by the nodes over the protocol following a consensus mechanism in charge of the communication between peers or players, as it’s named by the node’s implementation.

These players are identified in the protocol by certificates following a CA-CSR model. As this is controlled by the node, the tss-lib just receive the required parameters to work such as a player’s array, which are the peers that will take part in the keygen and signature process in the TSS protocol.

Therefore, the node is in charge of applying the necessary sanity checks for the TSS to work, and in this case, the node initialize keygen and signature processes without verifying whether each player is unique in the array. This could lead to further issues if any of the aforementioned processes start, taking into the account that repeated players in the array will have more power in the process depending on the specified threshold.

https://github.com/lombard-finance/consortium-node/blob/3eff3a4c9050e93c68a5aaf1dd1e34dfee3f51b7/shared/crypto/internal/eddsa_ed25519/keygen.go#L35-L48

func NewTEddsaCryptoKeygen(players []*types.Player, privateKey *ecdsa.PrivateKey, hostId uuid.UUID, crypto types.Crypto) (internal.ICryptoKeygen, error) {
	var curve elliptic.Curve
	if crypto == types.Crypto_CRYPTO_EDDSA_ED25519 {
		curve = tss.Edwards()
	} else {
		return nil, fmt.Errorf("there is no keygen for crypto (%s)", crypto.String())
	}
	return &teddsaCryptoKeygen{
		players:    players,
		privateKey: privateKey,
		hostId:     hostId,
		curve:      curve,
	}, nil
}

https://github.com/lombard-finance/consortium-node/blob/afad8f7bf90d6c2f27efb77e87de20cb4f84a80a/shared/crypto/internal/eddsa_ed25519/signer.go#L37-L45

func NewTEddsaCryptoSigner(players []*types.Player, privateKey *ecdsa.PrivateKey, hostId uuid.UUID, privateShare []byte, crypto types.Crypto) (internal.ICryptoSigner, error) {
	var curve elliptic.Curve
	if crypto == types.Crypto_CRYPTO_EDDSA_ED25519 {
		curve = tss.Edwards()
	} else {
		return nil, fmt.Errorf("there is no signer for crypto (%s)", crypto.String())
	}
	return &teddsaCryptoSigner{players: players, privateKey: privateKey, hostId: hostId, privateShare: privateShare, curve: curve}, nil
}

https://github.com/lombard-finance/consortium-node/blob/3eff3a4c9050e93c68a5aaf1dd1e34dfee3f51b7/shared/crypto/internal/ecdsa_secp256x1/keygen.go#L34-L49

func NewTssCryptoKeygen(players []*types.Player, privateKey *ecdsa.PrivateKey, hostId uuid.UUID, crypto types.Crypto) (internal.ICryptoKeygen, error) {
	var curve elliptic.Curve
	if crypto == types.Crypto_CRYPTO_ECDSA_SECP256K1 {
		curve = btcec.S256()
	} else if crypto == types.Crypto_CRYPTO_ECDSA_SECP256R1 {
		curve = elliptic.P256()
	} else {
		return nil, fmt.Errorf("there is no keygen for crypto (%s)", crypto.String())
	}
	return &TssCryptoKeygen{
		players:    players,
		privateKey: privateKey,
		hostId:     hostId,
		curve:      curve,
	}, nil
}

https://github.com/lombard-finance/consortium-node/blob/afad8f7bf90d6c2f27efb77e87de20cb4f84a80a/shared/crypto/internal/ecdsa_secp256x1/signer.go#L38-L54

func NewTssCryptoSigner(players []*types.Player, privateKey *ecdsa.PrivateKey, hostId uuid.UUID, privateShare []byte, curveType types.Crypto) (internal.ICryptoSigner, error) {
	var curve elliptic.Curve
	if curveType == types.Crypto_CRYPTO_ECDSA_SECP256K1 {
		curve = btcec.S256()
	} else if curveType == types.Crypto_CRYPTO_ECDSA_SECP256R1 {
		curve = elliptic.P256()
	} else {
		return nil, fmt.Errorf("there is no keygen for crypto (%s)", curveType.String())
	}
	return &TssCryptoSigner{
		players:      players,
		privateKey:   privateKey,
		hostId:       hostId,
		privateShare: privateShare,
		curve:        curve,
	}, nil
}

During the initialization of each process of the TSS, the uniqueness of each player should be verified to prevent from repeated entries.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The controller functions for handling replies in various consensus ceremonies — such as generating CA certificates, deposit addresses, issuing CSR certificates, notarizing transactions, and notarizing transaction outputs — are crucial for maintaining the integrity and security of these processes. These functions currently validate the structure and content of incoming replies and progress the consensus state based on these messages. However, they do not verify whether the incoming reply message originates from a participant associated with the current threshold key used in the cryptographic ceremony, nor do they check if the threshold key specified in the reply matches the one in the current consensus task's context (reply.GetThresholdKey() == c.thresholdKey.Id).

https://github.com/lombard-finance/consortium-node/blob/d4f7cc3d283f560abad43560cb02bfd51b4a668c/shared/consensus/internal/controller/generatecacertificate.go#L228-L248

func (c *generateCaCertificateController) HandleReply(reply *types.RequestProposal) (result *types.RequestProposal, err error) {
	// Validate the reply message
	reply2 := reply.GetGenerateCaCertificateReply()
	if reply2 == nil {
		return nil, fmt.Errorf("unable to handle incompatible reply message")
	}
	// Continue the signing ceremony with the received messages
	if c.cryptoSigner == nil {
		return nil, fmt.Errorf("CA signing ceremony hasn't started yet")
	}

	newReplies, res, err := c.cryptoSigner.Upgrade(reply2.Messages)
	if err != nil {
		return nil, err
	} else if res == nil {
		return c.createReplyResult(c.thresholdKey, reply, newReplies), nil
	}

	c.result = res
	c.caCert.Status = types.CertificateAuthorityStatus_CA_STATUS_GENERATED
	c.caCert.SerializedCert += "." + base64.RawURLEncoding.EncodeToString(res)

https://github.com/lombard-finance/consortium-node/blob/93caa5febffada61941b2891607f3ba5389c39a5/shared/consensus/internal/controller/generatedepositaddress.go#L175-L186

func (c *generateDepositAddressController) HandleReply(reply *types.RequestProposal) (result *types.RequestProposal, err error) {
	// do upgrade current keygen ceremony
	reply2 := reply.GetGenerateDepositAddressReply()
	if reply2 == nil {
		return nil, errors.New("unable to handle incompatible reply message")
	} else if err := validation.Validate(reply2); err != nil {
		return nil, err
	}
	if c.cryptoSigner == nil {
		log.Tracef("player (%s) is not in the party, skipping signing", c.consensusFeed.HostID())
		return nil, nil
	}

https://github.com/lombard-finance/consortium-node/blob/e373706ef06f91780179dafaed1d8822bc1bf40f/shared/consensus/internal/controller/issuecertificate.go#L337-L356

func (c *issueCsrCertificateController) HandleReply(reply *types.RequestProposal) (result *types.RequestProposal, err error) {
	// Validate the reply message
	reply2 := reply.GetIssueCsrCertificateReply()
	if reply2 == nil {
		return nil, fmt.Errorf("unable to handle incompatible reply message")
	} else if err = validation.Validate(reply2); err != nil {
		errors.Wrapf(err, "CSR HandleReply validation error!")
		return
	}

	// Continue the signing ceremony with the received messages
	if c.cryptoSigner == nil {
		return nil, fmt.Errorf("CSR signing ceremony hasn't started yet, , skipping reply")
	}
	newReplies, res, err := c.cryptoSigner.Upgrade(reply2.Messages) //  process replies and update the signing state
	if err != nil {
		return nil, err
	} else if res == nil {
		return c.createReplyResult(c.thresholdKey, reply, newReplies), nil
	}

https://github.com/lombard-finance/consortium-node/blob/ffed9c2720d84da657d91d2288d166d7b781a8a0/shared/consensus/internal/controller/notarizetransaction.go#L120-L135

func (c *notarizeTransactionController) HandleReply(reply *types.RequestProposal) (result *types.RequestProposal, err error) {
	// do upgrade current keygen ceremony
	reply2 := reply.GetNotarizeTransactionReply()
	if reply2 == nil {
		return nil, fmt.Errorf("unable to handle incompatible reply message")
	} else if err := validation.Validate(reply2); err != nil {
		return nil, err
	}
	if c.cryptoSigner == nil {
		log.Tracef("player (%s) is not in the party, skipping signing", c.consensusFeed.HostID())
		return nil, nil
	}
	newReplies, res, err := c.cryptoSigner.Upgrade(reply2.Messages)
	if err != nil {
		return nil, err
	}

https://github.com/lombard-finance/consortium-node/blob/57ea1b56098f1cd3a351f93e859a3036f1c2de3d/shared/consensus/internal/controller/notarizetransactionoutput.go#L150-L161

func (c *notarizeTransactionOutputController) HandleReply(reply *types.RequestProposal) (result *types.RequestProposal, err error) {
	// do upgrade current keygen ceremony
	reply2 := reply.GetNotarizeTransactionOutputReply()
	if reply2 == nil {
		return nil, errors.Errorf("unable to handle incompatible reply message")
	} else if err := validation.Validate(reply2); err != nil {
		return nil, err
	}
	if c.cryptoSigner == nil {
		log.Tracef("player (%s) is not in the party, skipping signing", c.consensusFeed.HostID())
		return nil, nil
	}

It is recommended to verify if reply sender is part of the current consensus task, and if the specified threshold key in the reply match with the one used in the current context.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The update function in the Service struct processes proposal requests, replies, and results within a consensus mechanism. However, it does not verify if the proposal associated with a request, reply, or result is the same as the one currently being processed in the consensus. This oversight can lead to inconsistencies and errors in the consensus process. Specifically, the function parses incoming messages and handles them without ensuring that the proposals are matched to the correct context.

This can result in processing outdated or irrelevant proposals, leading to potential consensus failures or data integrity issues.

To ensure reliable and accurate consensus handling, the function should be updated to verify that each incoming request, reply, or result corresponds to the currently active proposal.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L596-L628

} else if req.Reply != nil {
	var proposal *types.Proposal
	id, err := uuid.Parse(req.Id)
	if err != nil {
		log.Warnf("failed to parse proposal id (%s)", req.Id)
		return nil
	}
	if proposal = s.databaseService.GetProposalOrNil(id); proposal == nil {
		log.Warnf("there is no proposal (%s) to process reply, skipping", id)
		return nil
	}
	if err := s.handleJobReply(proposal, req); err != nil {
		log.Warnf("failed to handle proposal reply: %+v", err)
		return s.propose(common.RequestProposalError(proposal, err))
	}
}
if req.Result != nil || req.Error != "" {
	var proposal *types.Proposal
	id, err := uuid.Parse(req.Id)
	if err != nil {
		log.Warnf("failed to parse proposal id (%s)", req.Id)
		return nil
	}
	if proposal = s.databaseService.GetProposalOrNil(id); proposal == nil {
		log.Warnf("there is no proposal (%s) to process result, skipping", id)
		return nil
	}
	err = s.handleJobResult(proposal, req, entry)
	if err != nil {
		log.Warnf("failed to handle proposal result: %+v", err)
		return s.propose(common.RequestProposalError(proposal, err))
	}
}

As it was aforementioned, the update function should verify if the proposal used in the request is the same as the one being processed by the current consensus process, in that case, the request should be reject to prevent from further issues.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The checkProposalIsWellFormed function is intended to validate proposals by ensuring that exactly one non-nil value is present among the provided values. The function's logic is flawed, as it incorrectly returns true for empty proposals, thereby treating them as well-formed. This behavior occurs because the function does not explicitly check for the presence of at least one non-nil value before returning true.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L317-L328

func checkProposalIsWellFormed(values ...interface{}) bool {
	hasValue := false
	for _, v := range values {
		if v == nil {
			continue
		} else if hasValue {
			return false
		}
		hasValue = true
	}
	return true
}

In this implementation, if an empty list of values is passed, the loop is skipped entirely, and the function returns true, indicating that the empty proposal is well-formed. This logical oversight allows empty proposals to bypass validation checks, potentially leading to significant security vulnerabilities.

To address the issue in the checkProposalIsWellFormed function, it is recommended that the function be modified to ensure that at least one non-nil value is explicitly checked before returning true.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

In the current implementation of the project, data structures used throughout the system are supposed to undergo validation checks as defined in validator.go to ensure they are well-formed before processing. This validation is crucial to maintaining the integrity and stability of the system, as it prevents malformed data from causing errors, inconsistencies, or security vulnerabilities during runtime. However, there appears to be an inconsistency in how and where these validations are applied, with some data structures not being validated at all before use.

This inconsistency arises because the validation routines are not uniformly integrated across all points where data structures are received, created, or modified. For instance, structures might be validated in one part of the application (such as at the entry point of data) but are assumed to be valid in subsequent operations without re-validation. This assumption can lead to serious issues if the data is inadvertently corrupted or modified between processes or if the initial validation fails to catch certain edge cases.

The list of unimplemented validations is the following:

• NotarizedTransactionOutput

• ThresholdKey

• GenerateThresholdKeyReply

• GenerateThresholdKeyResult

Also, the NotarizedTransaction object lacks from check in some crucial struct’s members:

func validateNotarizedTransaction(err *Error, tx *types.NotarizedTransaction) {
	validateField(err, "id", tx.Id, validation.Required, is.UUIDv4)
	validateEnum(err, "status", tx.Status, types.NotarizedTransactionStatus_value)
	validateEnum(err, "blockchain", tx.Blockchain, types.Blockchain_value)
	validateField(err, "transaction_hash", tx.TransactionHash, validation.Required, validation.Length(32, 32))
	validateField(err, "block_number", tx.BlockNumber, validation.Required, validation.Min(0))
	validateField(err, "block_hash", tx.BlockHash, validation.Required, validation.Length(32, 32))
	validateField(err, "receipt_hash", tx.ReceiptHash, validation.Required, validation.Length(32, 32))
	validateField(err, "transferred_amount", tx.TransferredAmount, validation.Required)
	validateField(err, "chain_id", tx.ChainId, validation.Required, validation.Length(1, 32))
	validateField(err, "proposal", tx.Proposal, validation.Required, is.UUIDv4)
}

type NotarizedTransaction struct {
	Id                   string                     `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"`
	Status               NotarizedTransactionStatus `protobuf:"varint,2,opt,name=status,proto3,enum=finance.lombard.node.NotarizedTransactionStatus" json:"status,omitempty"`
	Blockchain           BlockchainType             `protobuf:"varint,3,opt,name=blockchain,proto3,enum=finance.lombard.node.BlockchainType" json:"blockchain,omitempty"`
	TransactionHash      []byte                     `protobuf:"bytes,4,opt,name=transaction_hash,json=transactionHash,proto3" json:"transaction_hash,omitempty"`
	BlockNumber          uint64                     `protobuf:"varint,5,opt,name=block_number,json=blockNumber,proto3" json:"block_number,omitempty"`
	BlockHash            []byte                     `protobuf:"bytes,6,opt,name=block_hash,json=blockHash,proto3" json:"block_hash,omitempty"`
	TransactionIndex     uint64                     `protobuf:"varint,7,opt,name=transaction_index,json=transactionIndex,proto3" json:"transaction_index,omitempty"`
	ReceiptHash          []byte                     `protobuf:"bytes,8,opt,name=receipt_hash,json=receiptHash,proto3" json:"receipt_hash,omitempty"`
	TransferredAmount    string                     `protobuf:"bytes,9,opt,name=transferred_amount,json=transferredAmount,proto3" json:"transferred_amount,omitempty"`
	ChainId              uint64                     `protobuf:"varint,10,opt,name=chain_id,json=chainId,proto3" json:"chain_id,omitempty"`
	ThresholdKey         string                     `protobuf:"bytes,11,opt,name=threshold_key,json=thresholdKey,proto3" json:"threshold_key,omitempty"`
	Proposal             string                     `protobuf:"bytes,12,opt,name=proposal,proto3" json:"proposal,omitempty"`
	Payload              []byte                     `protobuf:"bytes,13,opt,name=payload,proto3" json:"payload,omitempty"`
	Signature            []byte                     `protobuf:"bytes,14,opt,name=signature,proto3" json:"signature,omitempty"`
	RawPayload           []byte                     `protobuf:"bytes,15,opt,name=raw_payload,json=rawPayload,proto3" json:"raw_payload,omitempty"`
	XXX_NoUnkeyedLiteral struct{}                   `json:"-"`
	XXX_unrecognized     []byte                     `json:"-"`
	XXX_sizecache        int32                      `json:"-"`
}

It is recommended to add validation for each object and object’s members used by the program in order to prevent for further issues and inconsistencies in its execution.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation, and stating:

Partially deprecated since NotarizeTransaction was changed to NotarizeBridgeDeposit.

The calcDestinationId function is designed to generate a UUID based on a destination address and a blockchain identifier, aiming to ensure a unique identifier for different blockchain interactions. The function appends a destination address (to) with a separator byte, then appends the bytes representing the blockchain identifier, and finally applies a Keccak256 hash to the concatenated data. While this method initially seems reliable, the deterministic nature of the hash function combined with the modification of certain bytes to conform to UUID version 4 specifications introduces the possibility of collisions. Specifically, the adjustments made to ensure the UUID version and variant can reduce the entropy of the resulting UUID.

https://github.com/lombard-finance/consortium-node/blob/93caa5febffada61941b2891607f3ba5389c39a5/shared/consensus/internal/controller/generatedepositaddress.go#L283-L291

func calcDestinationId(to []byte, chain types.Blockchain) uuid.UUID {
	data := append(to, byte(1)) // separator byte
	data = append(data, new(big.Int).SetInt64(int64(chain)).Bytes()...)
	data = gethcrypto.Keccak256(data)   // hash
	data[6] = (data[6] & 0x0f) | 0x40   // version is 4
	data[8] = (data[8] & 0x3f) | 0x80   // variant is 10
	res, _ := uuid.FromBytes(data[:16]) // use first 16 bytes for uuid
	return res
}

Moreover, using only the first 16 bytes of the hash limits the space of possible outcomes, which could lead to different inputs (different combinations of to and chain) producing the same UUID.

To address the potential for UUID collisions, consider implementing a more robust method of UUID generation that either increases the entropy available for each UUID or incorporates a wider range of input variations.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation, and stating:

This part of logic was deprecated and rewrote.

The Start function in the Service struct is responsible for initializing node managers for various blockchain configurations. However, it does not initialize some chainId values in the ChainIds mapping, leading to potential inconsistencies and errors. The function parses the configuration and initializes the Bitcoin node manager correctly. For EVM-based configurations, it iterates through config.EvmConfigs, creating node managers and logging their initialization. However, the ChainIds mapping is only updated for specific keys ("0x4268" and "0x01"), which are hardcoded to map to types.Blockchain_BLOCKCHAIN_ETHEREUM. This approach overlooks other potential blockchain configurations, resulting in incomplete mapping initialization. Consequently, the system may fail to recognize or correctly manage blockchain nodes with uninitialized chainId values, leading to operational failures.

https://github.com/lombard-finance/consortium-node/blob/7658f25e5dcc600a70c75a7b1a7240da63c14968/shared/blockchain/service.go#L27-L50

func (s *Service) Start(cp shared.IConfigProvider) error {
	config := &Config{}
	if err := cp.Parse(config); err != nil {
		return err
	}
	btc, err := newBtcNodeManager(config.BitcoinConfig)
	if err != nil {
		return errors.Wrap(err, "new btc node manager")
	}
	s.bitcoin = btc
	for key, nc := range config.EvmConfigs {
		nm, err := s.factoryNodeManager(nc.Blockchain, nc)
		if err != nil {
			return errors.Wrapf(err, "unable to factory node manager")
		}
		log.Infof("found blockchain node config (%s) with chain id (%d)", nc.Name, nc.ChainID.Uint64())
		s.nms[nc.ChainID.Uint64()] = nm

		if key == "0x4268" || key == "0x01" {
			s.ChainIds[types.Blockchain_BLOCKCHAIN_ETHEREUM] = nc.ChainID
		}
	}
	return nil
}

To resolve this, the function should be updated to ensure all relevant chainId values are initialized in the ChainIds mapping.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The following code snippet, located in GenerateThresholdKey function, is designed to retrieve a ThresholdKey from the database, continuously looping until the key is obtained:

https://github.com/lombard-finance/consortium-node/blob/c178d2a5ca3e454482e6703a4e948ff550d2c312/shared/gateway/private.go#L32-L39

var tk *types.ThresholdKey
for tk == nil {
	tk = s.databaseService.GetThresholdKeyOrNil(uuid.MustParse(p.Id))
	if tk != nil {
		break
	}
	time.Sleep(100 * time.Millisecond)
}

However, this implementation is flawed as it can lead to an infinite loop. If the GetThresholdKeyOrNil method continuously returns nil due to the key not being present in the database, the loop will never terminate. This can cause the system to hang indefinitely, consuming resources and potentially leading to application crashes or degraded performance.

It is recommended to use a timeout or restrict the number of iterations.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The provided code snippet is part of a loop handling responses from a Threshold Signature Scheme (TSS) and assembling a digital signature once all required parts are received. The loop listens for messages on two channels: s.messageC for intermediate TSS messages, which are then encrypted by s.encryptReplies, and s.signC for the final signature components. However, the current implementation lacks critical validation checks to ensure the integrity and validity of the data received through these channels. This omission could originate further issues where invalid or corrupted data could potentially be processed and used to construct a digital signature. For instance, there is no verification to ensure that the signature components R, S, and SignatureRecovery received from s.signC are valid. If these components are malformed or incorrect, the final signature would be invalid, potentially leading incorrect verification or authorization of transactions.

https://github.com/lombard-finance/consortium-node/blob/afad8f7bf90d6c2f27efb77e87de20cb4f84a80a/shared/crypto/internal/ecdsa_secp256x1/signer.go#L151-L170

for {
	select {
	case msg := <-s.messageC:
		replies, err = s.encryptReplies(replies, msg)
		if err != nil {
			err = errors.Wrapf(err, "failed to handle tss message for ECDSA signing")
			return
		}
		continue
	case signatureData := <-s.signC:
		sigBuf := bytes.NewBuffer([]byte{})
		sigBuf.Write(signatureData.R)
		sigBuf.Write(signatureData.S)
		sigBuf.Write(signatureData.SignatureRecovery)
		result = sigBuf.Bytes()
		return
	default:
		return
	}
}

To enhance the robustness and security of the signature generation process, it is recommended to implement checks for all data received from TSS messages. Specifically:

• Validate TSS Messages: Before processing messages from s.messageC, validate their format and integrity. Ensure that they conform to expected structures and contain all necessary data fields without corruption.

• Verify Signature Components: Before assembling the final signature from the data received on s.signC, validate the signature components (R, S, SignatureRecovery). Check that these components are within the correct cryptographic bounds and are not corrupted or malformed.

• Error Handling: Enhance error handling mechanisms to manage scenarios where invalid data is detected. Instead of merely logging the error, implement strategies to retry the signature process or abort it gracefully, notifying the relevant systems or users of the failure.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The Service struct contains several placeholder functions intended for DragonBoat consensus integration, including Lookup, Sync, PrepareSnapshot, SaveSnapshot, RecoverFromSnapshot, and Close. These functions currently return nil or perform no operations, leaving the consensus mechanism incomplete. This can lead to further issues such as consensus failures, data integrity problems, and operational instability due to the lack of proper state management and synchronization in some cases where these functions are involved.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L632-L661

func (s *Service) Lookup(interface{}) (interface{}, error) {
	return nil, nil
}

func (s *Service) Sync() error {
	return nil
}

func (s *Service) PrepareSnapshot() (interface{}, error) {
	return nil, nil
	//return s.databaseService.NewSnapshot(), nil
}

func (s *Service) SaveSnapshot(snapshot interface{}, writer io.Writer, _ <-chan struct{}) error {
	return nil
	//ss := snapshot.(database.IDatabaseSnapshot)
	//if err := ss.Stream(writer); err != nil {
	//	return err
	//}
	//return ss.Close()
}

func (s *Service) RecoverFromSnapshot(reader io.Reader, _ <-chan struct{}) error {
	return nil
	//return s.databaseService.RecoverFromSnapshot(reader)
}

func (s *Service) Close() error {
	return nil
}

To address this, each function must be fully implemented to handle its respective functionalities included in DragonBoat.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The update function in the Service class is a critical component responsible for processing various types of requests within a consensus mechanism, including proposals, replies, and results. This function parses and handles different types of messages based on the structure and content of the request. However, a notable omission in the implementation is the lack of validation checks using the checkProposalIsWellFormed function, which is utilized in other parts of the consensus system to ensure that proposals are well-formed and meet the required criteria before proceeding.

https://github.com/lombard-finance/consortium-node/blob/66dd816a4891c382123852976c67a3dd3ea1249e/shared/consensus/service.go#L317-L328

func checkProposalIsWellFormed(values ...interface{}) bool {
	hasValue := false
	for _, v := range values {
		if v == nil {
			continue
		} else if hasValue {
			return false
		}
		hasValue = true
	}
	return true
}

It is recommended to integrate the checkProposalIsWellFormed function into the update function. This integration should occur early in the message handling process to ensure that all incoming proposals are scrutinized for completeness and validity before any processing takes place.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The Start function in the Service struct initializes node managers for blockchain configurations. However, if the configuration contains duplicate entries with the same ChainID, the function may initialize multiple instances of the same node manager. This leads to resource wastage and potential overwriting of node managers, causing operational inconsistencies. Specifically, the loop in the function does not check if a node manager for a given ChainID already exists before creating a new one. This oversight can result in multiple initializations.

https://github.com/lombard-finance/consortium-node/blob/7658f25e5dcc600a70c75a7b1a7240da63c14968/shared/blockchain/service.go#L37-L48

	for key, nc := range config.EvmConfigs {
		nm, err := s.factoryNodeManager(nc.Blockchain, nc)
		if err != nil {
			return errors.Wrapf(err, "unable to factory node manager")
		}
		log.Infof("found blockchain node config (%s) with chain id (%d)", nc.Name, nc.ChainID.Uint64())
		s.nms[nc.ChainID.Uint64()] = nm

		if key == "0x4268" || key == "0x01" {
			s.ChainIds[types.Blockchain_BLOCKCHAIN_ETHEREUM] = nc.ChainID
		}
	}

To prevent multiple initializations, the function should check if a node manager for a given ChainID already exists before creating a new one.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The functions verifyBridgeDeposit and verifyUnstakeRequest in the web3NodeManager handle events emitted by a smart contract. In both functions, data is parsed from the event logs to extract important information such as toAddress, toContract, toChainId, amount, and other fields. While these inputs are retrieved from a trusted smart contract, there is currently no validation implemented to ensure that the extracted data follows the correct format and contains valid values. This lack of validation could lead to further issues if the data is malformed or unexpected, potentially causing errors downstream or leading to incorrect processing.

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/blockchain/web3.go#L108-L113

// parse data
// DepositToBridge (index_topic_1 address fromAddress, index_topic_2 bytes32 toAddress, bytes32 toContract, bytes32 chainId, uint64 amount)
toAddress := eventLog.Topics[2]
toContract := eventLog.Data[:32]
toChainId := eventLog.Data[32:64]
amount := new(big.Int).SetBytes(eventLog.Data[64:96])

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/blockchain/web3.go#L164-L167

// parse data
amount := new(big.Int).SetBytes(eventLog.Data[32:64])
length := new(big.Int).SetBytes(eventLog.Data[64:96])
hexOutput := eventLog.Data[96 : 96+length.Int64()]

Implement input validation checks for the parsed data fields in both functions. This includes verifying that the toAddress, toContract, toChainId, amount, length, and hexOutput fields conform to expected formats and value ranges. By adding these checks, the functions will be more robust, ensuring that only correctly formatted and valid data is processed, thus minimizing the risk of unexpected behavior or errors.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The SponsorTx function adjusts the change output of a Bitcoin transaction by deducting a calculated fee. However, the fee calculation within CalculateFee is currently flawed. The method doubles the estimated transaction size (tx.SerializeSize() * 2), leading to an imprecise fee estimation. This can result in an overestimation or underestimation of the required fee, which could cause the transaction to either be rejected by the network or overpay in fees, wasting resources. Additionally, the function checks if the remaining change output is above a certain threshold, but this logic might also be affected by the inaccurate fee calculation. If the calculated fee is too high, it may trigger unexpected errors, stating that the change output is too small when, in reality, it is sufficient.

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/bitcoin/utils.go#L280-L294

func SponsorTx(tx *wire.MsgTx, changeOutput *wire.TxOut, feePerVB int64) error {
	fee := CalculateFee(tx, feePerVB)

	if btcutil.Amount(changeOutput.Value) <= fee {
		return errors.Errorf("change output (%d) is too small (expectedFee=%d)", changeOutput.Value, fee)
	}
	changeOutput.Value = changeOutput.Value - int64(fee)

	dustLimit := GetDustLimitForOutput(changeOutput)
	if btcutil.Amount(changeOutput.Value) < dustLimit {
		return errors.WithMessagef(ErrTooSmallOutput, "%d", changeOutput.Value)
	}

	return nil
}

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/bitcoin/utils.go#L247-L253

func CalculateFee(tx *wire.MsgTx, feePerVB int64) btcutil.Amount {
	// TODO: tx.SerializeSize() doesn't return real size of transaction
	// TODO: we need to reimplement this method to estimate real vB size, for now just multiply by 2
	b := tx.SerializeSize()
	fee := feePerVB * int64(b) * 2
	return btcutil.Amount(fee)
}

Refactor the CalculateFee function to accurately estimate the transaction's virtual size (vB) rather than relying on the simple doubling of the serialized size.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

It’s been identified that there are some functions which implement some extra arguments that are not being using during these functions. Thus, increasing unnecessarily the complexity of the code.

https://github.com/lombard-finance/consortium-node/blob/0359c88fdd67e557c537a02887a16035d5c8b971/shared/database/service.go#L183-L189

func (s *Service) GetProposalsByStatusOrderByIndex(status types.ProposalStatus, offset, size uint32) []*types.Proposal {
	result := s.d.FindProposalsByStatusOrderByIndex(status)
	if result == nil {
		result = []*types.Proposal{}
	}
	return result
}

It’s recommended to remove those extra parameters if they are not going to be used in the function’s implementation.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

It has been identified Open TO-DOs in the scoped files. Open TO-DOs can point to architecture or programming issues that still need to be resolved. Often these kinds of comments indicate areas of complexity or confusion for developers. This provides value and insight to an attacker who aims to cause damage to the protocol.

Consider resolving the TO-DOs before deploying code to a production context. Use an independent issue tracker or other project management software to track development tasks.

Remediation Plan

PARTIALLY SOLVED: The Lombard team partially solved this issue by resolving some TO-DO comments and states:

We did review of all TO-DOs and added them to our backlog. 2 TO-DOs we going to fix in scope of audit, others is not critical for release.

In the verifyBridgeDeposit function of the web3NodeManager class, there's a comparison between the eventLog.Address and the nm.config.LBTCAddress using a native comparison operator (!=). This approach is not optimal because Ethereum addresses, which are complex data structures, should be compared using dedicated methods that are specifically designed to handle such comparisons correctly.

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/blockchain/web3.go#L104-L106

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/blockchain/web3.go#L160-L162

if eventLog.Address != ethcommon.BytesToAddress(nm.config.LBTCAddress) {
	return nil, errors.Errorf("wrong contract address (%s)", eventLog.Address.String())
}

Replace the native comparison with a method specifically designed for Ethereum address comparison. The common.Address class typically has an Cmp method or similar that should be used for this purpose.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.

The preHandle function within the signDepositController class is tasked with preparing the necessary components for a Bitcoin Partially Signed Bitcoin Transaction (PSBT) by iterating through UTXOs provided in a request. This function extracts and decodes UTXO details such as the outpoint, script, and amount to build the transaction inputs for the PSBT. However, a significant oversight in the implementation is the lack of a check to ensure that the total sum of the UTXOs equals the specified amount in the signDepositSigningData structure, inclusive of transaction fees. This total should accurately reflect the total outputs of the transaction, ensuring that there is no discrepancy between the funds being signed over and the funds available in the provided UTXOs.

https://github.com/lombard-finance/consortium-node/blob/7899a964ede9cd23aef10a8cb139205ef4989798/shared/consensus/internal/controller/sign_deposit.go#L191-L229

	signingData := &signDepositSigningData{
		amount:        amount,
		stakerPk:      strings.TrimPrefix(stakerPk.PublicKey, "0x"),
		fpPk:          req2.GetFinalityProviderPk(),
		feeRate:       btcutil.Amount(req2.FeeRate),
		stakingTime:   uint16(req2.GetStakingTime()),
		userId:        userId,
		psbt:          nil,
		changeScript:  changeScript,
		babylonParams: babylonParams,
	}

	// iterate through the UTXOs, checking that we can interpret them and
	// recording useful information for the PSBT
	for i, u := range req2.GetUtxos() {
		// input seqnum
		psbtSeqnums[i] = wire.MaxTxInSequenceNum

		outpoint, err2 := wire.NewOutPointFromString(u)
		if err2 != nil {
			return nil, signingData, errors.Wrapf(err2, "parse utxo %d to outpoint (%s)", i, u)
		}
		psbtTxIns[i] = outpoint

		utxo, err2 := c.blockchainService.GetUTXO(outpoint)
		if err2 != nil {
			return nil, signingData, errors.Wrapf(err2, "get utxo %d", i)
		}

		// compute the WitnessUtxo field for the PSBT
		scriptBytes, err2 := hex.DecodeString(utxo.ScriptPubKey.Hex)
		if err2 != nil {
			return nil, signingData, errors.Wrapf(err2, "Decoding ScriptPubKey %d '%v'", i, utxo.ScriptPubKey.Hex)
		}
		txAmount, err2 := btcutil.NewAmount(utxo.Value)
		if err2 != nil {
			return nil, signingData, errors.Wrapf(err2, "UTXO %d amount %f", i, utxo.Value)
		}
		psbtInputs[i].WitnessUtxo = wire.NewTxOut(int64(txAmount), scriptBytes)

While the consensus mechanism might eventually reject a transaction where the input sums do not match the expected outputs (including fees), relying solely on this downstream validation introduces unnecessary inefficiency. It could lead to situations where invalid transactions are only caught after significant processing, wasting resources and potentially delaying other tasks’ finalization.

It’s recommended to add a check which verifies whether the sum of input UTXOs is equal or greater than the specified amount parameter.

Remediation Plan

SOLVED: The Lombard team solved this issue following the aforementioned recommendation.