hats-finance / illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf

0 stars 1 forks source link

`WitnessPublicKeys` can not be set due to missing functionality #36

Open hats-bug-reporter[bot] opened 3 months ago

hats-bug-reporter[bot] commented 3 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xa2699d44be4821a5f8fa7ef6755c08cc5b7e7436b4566722d09b09fba5f77d85 Severity: medium

Description: Description\

TEERollup.sol is a base contract which is inherited by BitcoinProver.sol contract.

To set the witness public key, this base contract has implemented:

    function _setWitnessPublicKeys(WitnessActivation[] memory _witnesses) internal {
        for (uint i = 0; i < _witnesses.length; i++) {
            witnessPublicKeysSet[_witnesses[i].publicKey] = _witnesses[i].isActive;
        }
    }

However, this internal function is not used by BitcoinProver.sol contract to set the witness public keys. This means that, witness public keys can not be set in BitcoinProver contract.

It should be noted that. BitcoinProver.sol has implemented function to set minimum witness signature i.e setMinWitnessConfirmations() but missed to implement the setWitnessPublicKeys function.

This would affect the verifyComputations() function as it would revert for the given FullComputationsProof as it wont be able to verify the witness signatures.

BitcoinProver.sol must implement the setWitnessPublicKeys() in order to prevent its functionalities from breaking and the functionalities would be useless whereever they make used of witness public keys.

Recommendations\

Consider impelmenting setWitnessPublicKeys public function in BitcoinProver.sol contract.

0xRizwan commented 3 months ago

For example understanding:

Recommendation to fix:

+     function setWitnessPublicKeys(bytes[] calldata publicKeys) public onlyOwner {
+        TEERollup.WitnessActivation[] storage _witness = new TEERollup.WitnessActivation[](publicKeys.length);
+        for (uint i = 0; i < _witness.length; i++) {
+            _witness[i].publicKey = publicKeys[i];
+            _witness[i].isActive = true;
+        }
+        _setWitnessPublicKeys(_witness);
+    }
party-for-illuminati commented 3 months ago

https://github.com/hats-finance/illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/blob/3ad7c2aedf991493aab45d3e0847b7e07f5c0d07/packages/contracts/contracts/illuminex/xengine/chains/btc/BitcoinProver.sol#L89

0xRizwan commented 2 months ago

@party-for-illuminati toggleWitnessPublicKey() would be used to toggle the ALREADY set public keys which were either active or inactive.

and the above recommended setWitnessPublicKeys() public function would set the number of witness public keys by calling it once instead of repeating for each public key. Further, _setWitnessPublicKeys() as internal function has not been used across inscope contracts.

Can you please clarify how _setWitnessPublicKeys() is expected to be used in contracts and how multiple witness public keys will be set to active?