Horizen Migration - Code Review - The Horizen Foundation

All contracts that inherit from OpenZeppelin Ownable (EONBackupVault.sol, ZendBackupVault.sol, LinearTokenVesting.sol, ZenMigrationFactory.sol) employ the single-step transferOwnership(newOwner) pattern: the current owner designates the new owner, and the transfer is finalized within the same transaction.

A typographical or copy-paste error could irreversibly relinquish administrative control, rendering this pattern unsafe. It could be improved by implementing a two-step process to enhance security.

Use OpenZeppelin’s Ownable2Step contract instead of Ownable, replacing direct transferOwnership calls with a two-step handover process: first, call transferOwnership(newOwner), then require newOwner to invoke acceptOwnership() to complete the ownership transfer.

ACKNOWLEDGED: The Horizen Foundation team acknowledged this finding.

The Zend backup vault currently lacks a recovery mechanism to forward unclaimed tokens to the DAO or the foundation after the designated grace period.

Such unclaimed funds may result from lost wallets or errors during multisig creation, leading to tokens remaining permanently locked in the vault contract. Consequently, these tokens are effectively removed from the total supply of ZEN tokens.

Implement a recoverUnclaimed() function within the vault contract. This function should verify the block timestamp against the grace period and, once expired, transfer any remaining ZEN tokens to the DAO multisig.

ACKNOWLEDGED: The Horizen Foundation team acknowledged this finding, mentioning that residual balances in the vaults that are never claimed should remain locked forever, slightly reducing effective supply.

In the zend_to_horizen.py dump script the conversion function calls round(), but the call is redundant, multiplying by 10**10 already produces an integer for any satoshi amount:

SATOSHI_TO_WEI_MULTIPLIER = 10 ** 10

def satoshi_2_wei(value_in_satoshi):
	return int(round(SATOSHI_TO_WEI_MULTIPLIER * value_in_satoshi))

The extra rounding has no effect on the result and can be removed without altering precision or behaviour.

Eliminate the redundant round() call in satoshi_2_wei and refactor it to return SATOSHI_TO_WEI_MULTIPLIER * value_in_satoshi directly (or wrap this multiplication in int() if necessary) to preserve precision without unnecessary rounding.

ACKNOWLEDGED: The Horizen Foundation team acknowledged this finding.

In the zend_to_horizen.py dump script, the eon_vault_results variable, which tracks the balances in ZEND mapped to EON addresses, is sorted into the sorted_eon_vault_accounts variable. However, only the eon_vault_results is ultimately used to generate the intermediary artifact, which is then utilized by the setup_eon2_json.py script to produce the final address-to-value list.

if eon_vault_result_file_name is not None:
	sorted_eon_vault_accounts = collections.OrderedDict(sorted(eon_vault_results.items()))

	with open(eon_vault_result_file_name, "w") as jsonFile:
		json.dump(eon_vault_results, jsonFile, indent=4)

Update the json.dump call in zend_to_horizen.py to write sorted_eon_vault_accounts instead of eon_vault_results, ensuring the output artifact maintains the intended sorted order.

ACKNOWLEDGED: The Horizen Foundation team acknowledged this issue, mentioning that the intermediary artifact did not have to be sorted to be ingested properly by the setup_eon2_json.py script.

The VerificationLibrary.sol::parseZendSignature function accepts zero values for the signature's s and r parameters. Signatures with these components cause ecrecover to return address(0), which could pose a security concern if the resulting address is not properly validated. The verifyZendSignatureBool function reverts if the recovered address is zero; however, capturing the specific reason for such a zero address could provide clearer diagnostic information rather than a generic revert message.

function parseZendSignature(bytes memory hexSignature) internal pure returns (Signature memory){
    if(hexSignature.length != 65) revert SignatureMustBe65Bytes();
    bytes32 r;
    bytes32 s;
    uint8 v = uint8(hexSignature[0]);
    assembly {       
        r := mload(add(hexSignature, 33)) // bytes 1-32; the first byte is the recovery identifier
        s := mload(add(hexSignature, 65)) // bytes 33-65
    }
    // Rejects the “high-s” malleability twin (s′ = n − s) that signs the same message, to prevent S-malleability issues
    if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0)
        revert InvalidSignature(); // @audit should also revert if r or s are 0
    return Signature({r: r, s: s, v: v});
}

function verifyZendSignatureBool(bytes32 messageHash, Signature memory signature, bytes32 pubKeyX, bytes32 pubKeyY) internal pure returns(bool) {
    uint8 v_ethereumFormat;
    if (signature.v == 31 || signature.v == 32){
        // Zend signature from compressed pubkey has +4 in the first byte, but Ethereum expects this to be removed
        v_ethereumFormat = signature.v - 4;
    } else { // @audit consider explicitly checking that v is 27 or 28
        v_ethereumFormat = signature.v;
    }
    address msgSigner = ecrecover(messageHash, v_ethereumFormat, signature.r, signature.s);
    if (msgSigner == address(0)) revert InvalidSignature();

    // Generate an Ethereum address from the public key components
    bytes32 hash = keccak256(abi.encodePacked(pubKeyX, pubKeyY));
    address ethAddress = address(uint160(uint256(hash)));

    return msgSigner == ethAddress; 
}

Implement explicit non-zero checks in parseZendSignature by adding if (r == 0 || s == 0) revert InvalidSignature();. Additionally, enforce that signature.v == 27 || signature.v == 28 within verifyZendSignatureBool before calling ecrecover to prevent zero-address recoveries and enhance error diagnostics.

SOLVED: The Horizen Foundation team solved the issue in the specified commit, adding an assertion on the zero r and s values.

The VerificationLibrary.sol::verifyZendSignatureBool function supports two valid encodings for the recovery byte v:

1. Uncompressed format: v ∈ {27, 28}
   These values are directly accepted by the ecrecover precompile.

2. Compressed format: v ∈ {31, 32}
   Horizen encodes compressed public-key signatures by adding 4 to the standard value.
   The library subtracts 4 before invoking ecrecover.

function verifyZendSignatureBool(bytes32 messageHash, Signature memory signature, bytes32 pubKeyX, bytes32 pubKeyY) internal pure returns(bool) {
    uint8 v_ethereumFormat;
    if (signature.v == 31 || signature.v == 32){
        // Zend signature from compressed pubkey has +4 in the first byte, but Ethereum does not expect this
        v_ethereumFormat = signature.v - 4;
    }else{
        v_ethereumFormat = signature.v;
    }
    // ...
}

If signature.v does not fall within either {27, 28, 31, 32}, the function forwards the call to ecrecover. In this case, ecrecover will return address(0), resulting in a revert in the outer logic. However, this occurs only after incurring the cost of the precompile and without a clear error indication.

At the beginning of verifyZendSignatureBool, enforce that signature.v is one of the following values: 27, 28, 31, or 32 (e.g., require(v == 27 || v == 28 || v == 31 || v == 32, "Invalid v");). This ensures early reversion and prevents unsupported values from being passed to ecrecover.

SVOLED: The Horizen Foundation team solved the issue in the specified commit, adding an assertion on the allowed v range.