Holograph contest - ctf_sec's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L301 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L899 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L903

Vulnerability details

Impact

the impact is that if the operator failed to execute the job and the fallback operator failed to executor, after a user step in and execute the job, A non-operataor user get rewards, but cannot claim the reward.

function executeJob is called when the operator needs execute the job.

  function executeJob(bytes calldata bridgeInRequestPayload) external payable {

In the current implement, the operator is supposed to execute the job, if not the fallback operator can execute the job, if not, anyone (maybe a non-operator) can step in and execute the job

if (job.operator != msg.sender) {

uint256 elapsedTime = block.timestamp - uint256(job.startTimestamp);
uint256 timeDifference = elapsedTime / job.blockTimes;
/**
 * @dev validate that initial selected operator time slot is still active
 */
require(timeDifference > 0, "HOLOGRAPH: operator has time");
require(gasPrice >= tx.gasprice, "HOLOGRAPH: gas spike detected");

if (timeDifference < 6) {
  // require(fallbackOperator == msg.sender, "HOLOGRAPH: invalid fallback");
}

then afer timeDifference > 6, anyone can step in and execute the job

uint256 amount = _getBaseBondAmount(pod);

_bondedAmounts[job.operator] -= amount;

_bondedAmounts[msg.sender] += amount;

the msg.sender is rewarded

_bondedAmounts[msg.sender] += amount;

for a user to claim the reward, he needs to call

  function unbondUtilityToken(address operator, address recipient) external {

can he call this function? no he may not because of the check below

require(_bondedOperators[operator] != 0, "HOLOGRAPH: operator not bonded");
/**
 * @dev check if sender is not actual operator
 */
if (msg.sender != operator) {
  /**
   * @dev check if operator is a smart contract
   */
  require(_isContract(operator), "HOLOGRAPH: operator not contract");
  /**
   * @dev check if smart contract is oAwned by sender
   */
  require(Ownable(operator).isOwner(msg.sender), "HOLOGRAPH: sender not owner");
}

then the reward is stucked in the Operator contract because of the check _bondedOperators[operator] != 0, the msg.sender in this case, does not have to be a operator.

Proof of Concept

A job is created, operator failed to execute job, fallback operator does not execute the job.

A user gladly step in and call executeJob to get 1000 token as reward

_bondedAmounts[address(A)] += 1000;

but in the current state

_bondedOperators[operator] == 0

so he failed to call

function unbondUtilityToken(address operator, address recipient) external

because

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

Tools Used

Recommended Mitigation Steps

We recommend the project check if the executeJob caller is an operator in the first place

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

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

if (job.operator != address(0)) { 
// other logic
}

or push the reward for non-operator caller to a seperate map for user to claim.

Lines of code

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

Vulnerability details

Impact

The documentation has specification about slashing

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

If an operator acts maliciously, a percentage of their bonded HLG will get slashed. Misbehavior includes (i) downtime, (ii) double-signing transactions, and (iii) abusing transaction speeds. 50% of the slashed HLG will be rewarded to the next operator to execute the transaction, and the remaining 50% will be burned or returned to the Treasury.

num of Slashes / % Bond 1 4% 2 16% 3 36% 4 64% 5 100%

such slashing is not implemented in the code below

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;

and

/**
 * @dev check if slashed operator has enough tokens bonded to stay
 */
if (_bondedAmounts[job.operator] >= amount) {
  /**
   * @dev enough bond amount leftover, put operator back in
   */
  _operatorPods[pod].push(job.operator);
  _operatorPodIndex[job.operator] = _operatorPods[pod].length - 1;
  _bondedOperators[job.operator] = job.pod;
} else {
  /**
   * @dev slashed operator does not have enough tokens bonded, return remaining tokens only
   */
  uint256 leftovers = _bondedAmounts[job.operator];
  if (leftovers > 0) {
	_bondedAmounts[job.operator] = 0;
	_utilityToken().transfer(job.operator, leftovers);
  }

such slashing may slash more token uncessarily to not incentive the good-faith operator to maintain the network.

And such slashing may slash less token for malicious operator so the cost of violation is too low for bad actor to misbehave (failed to perform the job).

Proof of Concept

The implementation is missing the slashing mechanism, evidently.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project implement the slashing mechanism as speicifed by the documentation.