Incorrect Indexing of Offchain Signer Public Keys after update

[hats-bug-reporter]

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

Description:
Description
The VaultBitcoinWallet contract includes a mechanism to update the offchain signer's public key. However, the current implementation appends new public keys to an array (offchainSignerPubKeys) without removing or invalidating the old ones. This can lead to incorrect indexing of public keys in functions that directly depend on the length of the array to determine the index of the latest key. Specifically, functions that use uint256 _offchainPubKeyIndex = offchainSignerPubKeys.length - 1; may point to an incorrect public key if the array size is updated due to the addition of new keys.

Attack Scenario\

Initial Setup:
The contract owner sets an initial offchain signer public key.
This key is used for various operations within the contract.

Key Update:
The contract owner updates the offchain signer public key.
Each update appends a new key to the offchainSignerPubKeys array without removing the old ones.

Incorrect Indexing:
Functions like deriveChangeInfo and generateOrder use the length of the offchainSignerPubKeys array to determine the index of the latest key.
Due to the accumulation of old keys, the index might point to an incorrect or outdated key.

Security Risk:
If an old key is compromised and not invalidated, it could be used maliciously.
Functions that rely on the index of the public key might retrieve an outdated or compromised key, leading to security vulnerabilities.

Attachments

1. Proof of Concept (PoC) File

function generateOrder(
        address to,
        bytes memory _data,
        bytes32 _entropy
    ) public view returns (bytes memory orderData, bytes memory btcAddr) {
        uint256 _keyIndex = _ringKeys.length - 1;
        uint256 _offchainPubKeyIndex = offchainSignerPubKeys.length - 1;//@audit-no logic implementation for pubkey updates
        bytes32 userSeed = _random(_entropy);
        bytes memory recoveryData = abi.encode(_offchainPubKeyIndex, userSeed, to, _data);

        bytes20 _scriptHash = _keyDataToScriptHash(_offchainPubKeyIndex, _keyIndex, keccak256(recoveryData));

        (bytes memory _encryptedOrder,) = _encryptPayload(recoveryData);

        orderData = abi.encode(_keyIndex, _encryptedOrder);
        btcAddr = _generateAddress(_scriptHash, _isTestnet() ? TYPE_ADDR_P2SH_TESTNET : TYPE_ADDR_P2SH);
    }

function deriveChangeInfo(bytes32 seed)
    public
    override
    onlyAuthorisedSerializer
    returns (uint256 _rChangeSystemIdx, bytes20 _changeScriptHash)
{
    bytes32 _changeSecret = _deriveNextChangeSecret(seed);
    uint256 _offchainPubKeyIndex = offchainSignerPubKeys.length - 1;

    _rChangeSystemIdx = _changeWalletsSecrets.length;
    _changeScriptHash = _generateVaultScriptHashFromSecret(_offchainPubKeyIndex, _changeSecret);

    _changeSystemIdxToOffchainPubKeyIndex[_rChangeSystemIdx] = _offchainPubKeyIndex;

    changeSecretCounter++;
    _changeWalletsSecrets.push(_changeSecret);
}

2. Revised Code File (Optional)

• The current implementation of updateOffchainSignerPubKey in the VaultBitcoinWallet contract appends new keys to an array without removing old ones. This can lead to incorrect indexing of public keys in functions that depend on the length of the array to determine the index of the latest key. To mitigate these issues, implement a mechanism to remove or invalidate old keys when a new key is added. This ensures that the contract remains efficient, secure, and accurate in its key management.