Holograph contest - minhtrng's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L491-L511

Vulnerability details

Impact

Using block.number and block.timestamp as a source of randomness is commonly advised against, as the outcome can be manipulated by calling contracts. In this case a compromised layer-zero-endpoint would be able to retry the selection of the primary operator until the result is favorable to the malicious actor.

Proof of Concept

An attack path for rerolling the result of bad randomness might look roughly like this:

function attack(uint256 currentNonce, uint256 wantedPodIndex, uint256 numPods, uint256 wantedOperatorIndex, uint256 numOperators,  bytes calldata bridgeInRequestPayload) external{

    bytes32 jobHash = keccak256(bridgeInRequestPayload);

    //same calculation as in HolographOperator.crossChainMessage
    uint256 random = uint256(keccak256(abi.encodePacked(jobHash, currentNonce, block.number, block.timestamp)));

    require(wantedPodIndex == random % numPods)
    require(wantedOperatorIndex == random % numOperators);

    operator.crossChainMessage(bridgeInRequestPayload);
}

The attack basically consists of repeatedly calling the attack function with data that is known and output that is wished for until the results match and only then continuing to calling the operator.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a decentralized oracle for the generation of random numbers, such as Chainlinks VRF.

It should be noted, that in this case there is a prerequirement of the layer-zero endpoint being compromised, which confines the risk quite a bit, so using a normally unrecommended source of randomness could be acceptable here, considering the tradeoffs of integrating a decentralized oracle.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L374-L382 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L849-L857

Vulnerability details

Impact

Bond tokens (HLG) equal to the slash amount will get permanently stuck in the HolographOperator each time a job gets executed by someone who is not an (fallback-)operator.

Proof of Concept

The HolographOperator.executeJob function can be executed by anyone after a certain passage of time:

...
if (job.operator != address(0)) {
    ...
    if (job.operator != msg.sender) {
        //perform time and gas price check
        if (timeDifference < 6) {
            // check msg.sender == correct fallback operator
        }
        // slash primary operator
        uint256 amount = _getBaseBondAmount(pod);
        _bondedAmounts[job.operator] -= amount;
        _bondedAmounts[msg.sender] += amount;

        //determine if primary operator retains his job
        if (_bondedAmounts[job.operator] >= amount) {
            ...
        } else {
            ...
        }
    }
}
// execute the job

In case if (timeDifference < 6) { gets skipped, the slashed amount will be assigned to the msg.sender regardless if that sender is currently an operator or not. The problem lies within the fact that if msg.sender is not already an operator at the time of executing the job, he cannot become one after, to retrieve the reward he got for slashing the primary operator. This is because the function HolographOperator.bondUtilityToken requires _bondedAmounts to be 0 prior to bonding and hence becoming an operator:

require(_bondedOperators[operator] == 0 && _bondedAmounts[operator] == 0, "HOLOGRAPH: operator is bonded");

Tools Used

Manual Review

Recommended Mitigation Steps

Assuming that it is intentional that non-operators can execute jobs (which could make sense, so that a user could finish a bridging process on his own, if none of the operators are doing it): remove the requirement that _bondedAmounts need to be 0 prior to bonding and becoming an operator so that non-operators can get access to the slashing reward by unbonding after.

Alternatively (possibly preferrable), just add a method to withdraw any _bondedAmounts of non-operators.

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L499-L511

Vulnerability details

Impact

The documents which contain the specification and were provided as part of the contest specify on multiple places that the likelihood to being selected as the primary operator for a job should be dependent on the number of tokens bonded:

https://docs.holograph.xyz/holograph-protocol/operator-network-specification#operator-job-selection :

• operators must join a pod. Each pod provides a different probability of selection, based on increasingly higher minimum bond requirements

https://github.com/code-423n4/2022-10-holograph/blob/main/README_DETAILS.md#operating

• Operators must run the CLI, bond the protocol’s native token, and execute transactions. The probability of getting selected to perform this work is based on the number of tokens bonded

This is currently not the case, as the amount of bonded tokens does not affect the random selection mechanism.

Proof of Concept

The random selection mechanism can be found in HolographOperator:

uint256 random = uint256(keccak256(abi.encodePacked(jobHash, _jobNonce(), block.number, block.timestamp)));
/**
 * @dev divide by total number of pods, use modulus/remainder
 */
uint256 pod = random % _operatorPods.length;
/**
 * @dev identify the total number of available operators in pod
 */
uint256 podSize = _operatorPods[pod].length;
/**
 * @dev select a primary operator
 */
uint256 operatorIndex = random % podSize;

Pods are essentially just selected by an equal distribution (and so are the operators within the pods). For a simple example of how this does not check out according to the specification, look at the following example:

• pod#1 has 1 operator
• pod#2 has 1 operator
• pod#3 has 2 operators

A higher pod number requires a higher bonding amount (details are in the docs and HolographOperator.getPotBondAmounts), so the operators within pod #3 should have a higher chance of being selected than the ones in the other pods. With this implementation they actually have a lower chance (~16.67%) of being selected than the operators in pod#1 and pod#2 (both ~33.33%)

It is presumed that the project has chosen this design to rely on rational market forces to adjust the distribution of operators over the pods.

If all operators were technically savvy, they would read the code, understand the selection mechanism and would adjust the pod they are in accordingly. This however can not be assumed and regular users would just try to get into the highest pod possible with the amount of bonding tokens they own, which can lead to operators in pods with lower bonding requirements actually having a higher chance of being selected due to higher pods being more "crowded" (as in the example above).

Tools Used

Manual Review

Recommended Mitigation Steps

Change the random selection mechanism to weight pods differently, depending on how many tokens are bonded for each particular pod, for example weight = minimum bonding requirement * amount of operators in the pod.