Holograph contest - csanuragjain's results

Lines of code

https://github.com/holographxyz/holograph-protocol/blob/c4_audit/contracts/HolographOperator.sol#L849

Vulnerability details

Impact

User can call bondUtilityToken function with large pod value which ensure pushing [address(0)] from _operatorPods.length till pod.

This becomes a problem _operatorPods.length increases dramatically causing randomess to reduce significantely in crossChainMessage. Reason being random % _operatorPods.length will mostly result in unused pod with address(0) as only operator, causing open season where anyone can execute the job

Proof of Concept

1. User A calls the bondUtilityToken function with pod say 100

  function bondUtilityToken(
    address operator,
    uint256 amount,
    uint256 pod
  ) external {
...
for (uint256 i = _operatorPods.length; i <= pod; i++) {
          /**
           * @dev add zero address into pod to mitigate empty pod issues
           */
          _operatorPods.push([address(0)]);
        }
...
}

2. Lets say _operatorPods initially had 2 elements so after calling this function and pod as 100 this will push address(0) 98 times making the _operatorPods length as 100

3. This becomes a problem while calling crossChainMessage function

function crossChainMessage(bytes calldata bridgeInRequestPayload) external payable {
...
uint256 pod = random % _operatorPods.length;
uint256 podSize = _operatorPods[pod].length;
 uint256 operatorIndex = random % podSize;
...
}

4. Assume random is selected as 10, which makes pod as 10 and _operatorPods[pod].length is 1 (while initializing one dummy operator with address 0 is added). This makes operatorIndex as 0 (random%1=0)

5. Now 0 operatorIndex means open season and anyone can execute the job

/**
       * @dev If operator index is 0, then it's open season! Anyone can execute this job. First come first serve
       *      pop operator to ensure that they cannot be selected for any other job until this one completes
       *      decrease pod size to accomodate popped operator
       */

Recommended Mitigation Steps

Do not allow user to specify a very large value for pods

Lines of code

https://github.com/holographxyz/holograph-protocol/blob/c4_audit/contracts/HolographOperator.sol#L382

Vulnerability details

Impact

In case if a non operator calls the executeJob, then the slashed amount added to this non operator cannot be redeemed as shown in poc causing loss of funds

Proof of Concept

1. Operator A forgets to execute job

2. User A sees an opportunity and immediately calls the executeJob function

3. This slashes the _getBaseBondAmount from Operator A and add this to User A

uint256 amount = _getBaseBondAmount(pod);
        /**
         * @dev select operator that failed to do the job, is slashed the pod base fee
         */
        _bondedAmounts[job.operator] -= amount;
        /**
         * @dev the slashed amount is sent to current operator
         */
        _bondedAmounts[msg.sender] += amount;

4. Now User A has _bondedAmounts[User A]=amount but _bondedOperators[User A] remain 0 since User A is not an operator

5. This becomes a problem since now User cannot redeem the funds using unbondUtilityToken function since below condition will fail

require(_bondedOperators[operator] != 0, "HOLOGRAPH: operator not bonded");

Recommended Mitigation Steps

Add a check to verify that caller is a valid operator

require(_bondedOperators[msg.sender] != 0, "HOLOGRAPH: operator not bonded");

Init function can be called by any user

Contract: https://github.com/holographxyz/holograph-protocol/blob/c4_audit/contracts/abstract/ERC20H.sol#L140 https://github.com/holographxyz/holograph-protocol/blob/c4_audit/contracts/abstract/ERC721H.sol#L140

Issue: It was observed that init function can be called by any user bricking the contract owner. This will force the contract owner to redeploy this contract causing wastage of gas

Recommendation: Only the owner (decided via constructor) should be allowed to call init function. This need to be fixed for almost all contracts