Description

The getValidatorIndex of the ValidatorManager Mina contract is intended to be used to check whether the passed public key is the public key of one of the three configured legitimate validators, and if so, which one. It returns the index of the legitimate validator, starting from 1, or 0 if the public key is not one of the three legitimate validators. The function is implemented as follows:

public getValidatorIndex(p: PublicKey): Field {
	if (this.compareValidators(p, this.validator1.getAndRequireEquals())) return Field.from(1);
  if (this.compareValidators(p, this.validator2.getAndRequireEquals())) return Field.from(2);
  if (this.compareValidators(p, this.validator3.getAndRequireEquals())) return Field.from(3);
  return Field.from(0);
}

The result of this.compareValidators(p, this.validator1.getAndRequireEquals()) is a Bool, a circuit variable that holds a Boolean value, not a TypeScript Boolean. Such a value is always truthy for TypeScript (see this section↗ of the Mina documentation), so the function will return Field.from(1), independently of the public key passed as an argument.

Impact

The function getValidatorIndex will, for every public key, return a value that indicates that this public key is the first legitimate validator. Thus, any key may be used and passed off as a validator, bypassing any checks involving validator signatures.

In the current version of the Bridge contract, getValidatorIndex is never called. This is however only due to another bug; see Finding ref↗. To avoid the impact discussed in Finding ref↗, both that bug as well as the one discussed in this finding will need to be fixed.

Recommendations

TypeScript if is not compatible with constructing proof circuits.

One option would be to make the getValidatorIndex function return (pseudocode),

comp1 * 1 + comp2 * 2 + comp3 * 3

where comp1 = this.compareValidators(p, this.validator1.getAndRequireEquals(), and similarly for comp2 and comp3. This would work as intended as long as the three validator public keys are pairwise distinct, as this assumption implies that at most one of the three Booleans can be nonzero.

Remediation

This issue has been acknowledged by Sotatek, and a fix was implemented in commit.