Holograph contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

According to the documentation, the operator is in charge of executing the job. If the job failed and reverted, the job can be redone with additional fee

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

Failed jobs can be re-done (for an additional fee), can be returned to origin chain (for an additional fee), or left untouched entirely. This shifts the financial responsibility towards users, rather than operators.

However, in the code, in the failed job left untouched and the re-done feature is not implemented. There are two places in the code that involve failed job

/**
* @dev Internal mapping of operator job details for a specific job hash
*/
mapping(bytes32 => bool) private _failedJobs;

and

try
  HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
	msg.sender,
	bridgeInRequestPayload
  )
{
  /// @dev do nothing
} catch {
  _failedJobs[hash] = true;
  emit FailedOperatorJob(hash);
}

the impact is that the failed job is discarded ant the cross-chain message failed to complete its cycle.

Proof of Concept

When user make a cross-chain call, the job is created by calling

  function crossChainMessage(bytes calldata bridgeInRequestPayload) external payable {

which set the job

_operatorJobs[jobHash] = uint256(
((pod + 1) << 248) |
  (uint256(_operatorTempStorageCounter) << 216) |
  (block.number << 176) |
  (_randomBlockHash(random, podSize, 1) << 160) |
  (_randomBlockHash(random, podSize, 2) << 144) |
  (_randomBlockHash(random, podSize, 3) << 128) |
  (_randomBlockHash(random, podSize, 4) << 112) |
  (_randomBlockHash(random, podSize, 5) << 96) |
  (block.timestamp << 16) |
  0
);

then a operator step in and execute the job, calling

HolographOperatorInterface(address(this)).nonRevertingBridgeCall{value: msg.value}(
msg.sender,
bridgeInRequestPayload
)

ok for example, the bridgeInRequestPayload eventually calls to via

Operator.sol#nonRevertingBridgeCall calls HolographBridge.sol#bridgeInRequest then calls HolographERC20.sol

function bridgeIn(uint32 fromChain, bytes calldata payload) external onlyBridge returns (bytes4) {
(address from, address to, uint256 amount, bytes memory data) = abi.decode(
  payload,
  (address, address, uint256, bytes)
);
_mint(to, amount);
if (_isEventRegistered(HolographERC20Event.bridgeIn)) {
  require(SourceERC20().bridgeIn(fromChain, from, to, amount, data), "HOLOGRAPH: bridge in failed");
}
return Holographable.bridgeIn.selector;
}

but the function revert in

require(SourceERC20().bridgeIn(fromChain, from, to, amount, data), "HOLOGRAPH: bridge in failed");

then catch block steps in, the _failedJobs is discarded and not able to be executed again.

 catch {
  _failedJobs[hash] = true;
  emit FailedOperatorJob(hash);
}

Tools Used

Manul Review

Recommended Mitigation Steps

We recommend the user implement the feature in the code.

Failed jobs can be re-done (for an additional fee), can be returned to origin chain (for an additional fee), or left untouched entirely. This shifts the financial responsibility towards users, rather than operators.

Lines of code

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

Vulnerability details

Impact

According to the documentation

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

In order to make pod and operator selection mathematically random, a random number is generated by running: sha3(sha3(jobPayload) + jobNonce + blockNumber + blockTimestamp)

the implementation is

  /**
   * @dev use job hash, job nonce, block number, and block timestamp for generating a random number
   */
  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;

however, the miner can affect the randomness in crossChainMessage.sol by manipulating the block.timestamp. Then the operator will not be fairly selected. The result and impact could be that a small list of operator get to execute the job and get the reward while other operator sit idle.

Proof of Concept

Source

https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/timestamp-dependence/

Timestamp Manipulation¶

Be aware that the timestamp of the block can be manipulated by a miner. Consider this contract:

uint256 constant private salt =  block.timestamp;

function random(uint Max) constant private returns (uint256 result){
    //get the best seed for randomness
    uint256 x = salt * 100/Max;
    uint256 y = salt * block.number/(salt % 5) ;
    uint256 seed = block.number/3 + (salt % 300) + Last_Payout + y;
    uint256 h = uint256(block.blockhash(seed));

    return uint256((h / x)) % Max + 1; //random number between 1 and Max
}

When the contract uses the timestamp to seed a random number, the miner can actually post a timestamp within 15 seconds of the block being validated, effectively allowing the miner to precompute an option more favorable to their chances in the lottery. Timestamps are not random and should not be used in that context.

In the context of the Holograph,

 uint256 random = uint256(keccak256(abi.encodePacked(jobHash, _jobNonce(), block.number, block.timestamp)));

jobHash is known, the miner can see the _jobNonce() for such, block.number is known, the minter can affect block.timestamp, then it can manipulate the random number generated.

Tools Used

Manual Review

Recommended Mitigation Steps

I want to quote the consensys page again

https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/timestamp-dependence/

The 15-second Rule If the scale of your time-dependent event can vary by 15 seconds and maintain integrity, it is safe to use a block.timestamp.

given that the block.timestamp can affect the randoness directly, the 15 seconds does not apply, so it is not safe to use block.timestamp.

We recommend the project consider other random solution such as

https://docs.chain.link/docs/vrf/v2/introduction/

Lines of code

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#L920 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L924 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L928 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L932

Vulnerability details

Impact

When user call unbondUtilityToken to unstake the token,

the function read the available bonded amount, and transfer back to the operator

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

/**
 * @dev get current bonded amount by operator
 */
uint256 amount = _bondedAmounts[operator];
/**
 * @dev unset operator bond amount before making a transfer
 */
_bondedAmounts[operator] = 0;
/**
 * @dev remove all operator references
 */
_popOperator(_bondedOperators[operator] - 1, _operatorPodIndex[operator]);
/**
 * @dev transfer tokens to recipient
 */
require(_utilityToken().transfer(recipient, amount), "HOLOGRAPH: token transfer failed");

the logic is clean, but does not conform to the buisness requirement in the documentation, the doc said

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

To move to a different pod, an Operator must withdraw and re-bond HLG. Operators who withdraw HLG will be charged a 0.1% fee, the proceeds of which will be burned or returned to the Treasury.

The charge 0.1% fee is not implemented in the code.

there are two incentive for bounded operator to stay,

the first is the reward incentive, the second is to avoid penalty with unbonding.

Without chargin the unstaking fee, the second incentive is weak and the operator can unbound or bond whenver they want

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

we recommend charge the 0.1% unstaking fee to make the code align with the busienss requirement in the doc.

/**
 * @dev get current bonded amount by operator
 */
uint256 amount = _bondedAmounts[operator];
uint256 fee = chargedFee(amount); // here
amount -= fee;  
/**
 * @dev unset operator bond amount before making a transfer
 */
_bondedAmounts[operator] = 0;
/**
 * @dev remove all operator references
 */
_popOperator(_bondedOperators[operator] - 1, _operatorPodIndex[operator]);
/**
 * @dev transfer tokens to recipient
 */
require(_utilityToken().transfer(recipient, amount), "HOLOGRAPH: token transfer failed");