Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FC4-04 | Possible Inconsistencies Between Sifchain States And Smart Contract States #3153

Open
Brando753 opened this issue Aug 23, 2022 · 2 comments
Assignees
Labels
Major Reported as a major Vulnerability Peggy Team Peggy team task Peggy 2.0 An issue blocking the Peggy 2.0 release

Comments

@Brando753
Copy link
Contributor

Let's huddle to discuss the best way to resolve this issue. @banshee @smartyalgo @Brando753

Description

Events emitted by the smart contracts will be synced to the Sifchain, and events on the Sifchain will also be synced to the smart contracts after collecting enough validator signatures, so smart contract states, such as the power of validators, consensus rate, and blacklist, should be consistent with Sifchain chain states after syncing. However, due to the latency caused by the syncing process, the smart contract states and the Sifchain states are not always consistent.

For example, the relayer checks if a event has collected enough validator signatures on the Sifchain:

func (k Keeper) processCompletion(ctx sdk.Context, networkDescriptor types.NetworkDescriptor, prophecy types.Prophecy) types.Prophecy {
	whiteList := k.GetOracleWhiteList(ctx, types.NewNetworkIdentity(networkDescriptor))
	voteRate := whiteList.GetPowerRatio(prophecy.ClaimValidators)
	var consensusNeeded float64
	consensusNeededUint, err := k.GetConsensusNeeded(ctx, types.NewNetworkIdentity(networkDescriptor))
	
	if err != nil {
		consensusNeeded = k.consensusNeeded
	} else {
		consensusNeeded = float64(consensusNeededUint) / 100.0
	}

	if voteRate >= consensusNeeded {
		prophecy.Status = types.StatusText_STATUS_TEXT_SUCCESS
	}

	instrumentation.PeggyCheckpoint(ctx.Logger(), instrumentation.ProcessCompletion, "prophecy", zap.Reflect("prophecy", prophecy))

	return prophecy
}

After the event is submitted to the Ethereum chain, the signatures will be checked again, include the validity of the signer's identity, the legitimacy of the signature and the consensus rate:

function getSignedPowerAndFindDup(SignatureData[] calldata _validators, bytes32 hashDigest)
    private view returns (uint256 pow){
    uint256 validatorLength = _validators.length; 
    for (uint256 i; i < validatorLength; ) {
      SignatureData calldata validator = _validators[i];
      require(isActiveValidator(validator.signer), "INV_SIGNER");
      require(
        verifySignature(validator.signer, hashDigest, validator._v, validator._r, validator._s),
        "INV_SIG");

      unchecked {pow += getValidatorPower(validator.signer);}

      for (uint256 j = i + 1; j < validatorLength; ) {
        require(validator.signer != _validators[j].signer, "DUP_SIGNER");
        unchecked {
          ++j;
        }
      }
      unchecked {
        ++i;
      }
    }
  }
function getProphecyStatus(uint256 signedPower) public view returns (bool) {
    // Prophecy must reach total signed power % threshold in order to pass consensus
    uint256 prophecyPowerThreshold = totalPower * consensusThreshold;
    // consensusThreshold is a decimal multiplied by 100, so signedPower must also be multiplied by 100
    uint256 prophecyPowerCurrent = signedPower * 100;
    bool hasReachedThreshold = prophecyPowerCurrent >= prophecyPowerThreshold;
    return hasReachedThreshold;
  }

However, it is possible that the state of a validator is changed, for example, a new validator has been added to the validatorWhiteList on the Sifchain, but the change has not been synced to the Ethereum chain. Therefore, the signature check might be passed on the Sifchain but fail the checkup on the CosmosBridge contract.

Recommendation

Recommend checking if the corner cases caused by the aforementioned inconsistencies are acceptable, if it is not, modifying the syncing mechanism to ensure consistency.

@Brando753 Brando753 added Peggy Team Peggy team task Peggy2 Audit - Sifnode Issue Issue that came out of the Peggy2 audits that requires remediation by Sifnode Major Reported as a major Vulnerability and removed Peggy2 Audit - Sifnode Issue Issue that came out of the Peggy2 audits that requires remediation by Sifnode labels Aug 23, 2022
@Brando753 Brando753 added the Peggy 2.0 An issue blocking the Peggy 2.0 release label Sep 6, 2022
@smartyalgo smartyalgo self-assigned this Sep 16, 2022
@pandaring2you
Copy link
Contributor

To discuss with @Brando753

@smartyalgo
Copy link
Contributor

The solution to this is to ensure we can pause import on the evm smart contract, and export on the sifnoded side. Currently smart contract supports pausing, but sifnoded doesn't. See #2445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Major Reported as a major Vulnerability Peggy Team Peggy team task Peggy 2.0 An issue blocking the Peggy 2.0 release
Projects
None yet
Development

No branches or pull requests

3 participants