Holograph contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

By passing a tiny gas price and a gigantic gas limit when bridging out, the choosen operator will either have to call executeJob but it'd be to expensive, or will get slashed.

Proof of Concept

• Alice sets up an Holographer ERC20 with some heavily gas intensive functions
• Alice calls bridgeOutRequest(toChain, holographableContract, 1e12, 1) in HolographBridge
• Alice pays messagingModule.getHlgFee(toChain, gasLimit, gasPrice) which would be in the order of magnitude of gasLimit * gasPrice ~ 1e12 so not expensive
• Provided the remaining msg.value is sufficient, the messaging module will pass the message to the destination chain
• On the destination chain, crossChainMessage in HolographOperator is called, eventually selecting an operator and removing it from its pod, doing _bondedOperators[operator] = 0; here.
• While _bondedOperators[operator] = 0;, the operator is frozen and cannot unbound its funds. To undo this the job needs to be executed, but this would be expensive because of this line. The only choice for the operator is either to loose access to its funds, either to pay 1e12 gas or whatever the gasLimit set by Alice was to execute the job.

Recommended Mitigation Steps

In crossChainMessage, the gasPrice needs to be checked to ensure it is not too small.

[NC-01] Typo

Here: destiantion -> destination Here: destiantion -> destination

[NC-02] It is unclear if the function is supposed to be in the final version of the code

Here it states "NOT PART OF FINAL CODE !!!". However the function is in this version for the audit, which is a bit confusing.

[NC-03] Left TODOs

Here

[NC-04] Typo

Here has -> gas

[NC-05] Typo

Here give - given

[G-01] delete operation could be avoided

Here the array is pop() anyway so there is no need to manually delete the storage

[G-02] Use a mapping instead of arrays for _operatorPods

In HolographOperator, _operatorPods is a Multi-dimensional array. But it is used as a stack, and successful length modifications (pop then push) will set the storage back to 0, and then to non 0, which costs overall ~20k gas.

Instead, you could use a mapping to represent the array and store the length of the array separately. This way, when popping an element you don't need to clear the storage and every sequence of pop then push will save you 10k gas.

[G-03] Useless getters

What are the utility of all getters like getOperator? If it's only for off-chain use, you could read from the storage slot directly and save deployment costs.

[G-05] Some private variables could be constants

Assuming resetOperator is not part of the final code at it seems to be the intent of the dev team, some private variables could be switch to constants to save numerous cold SLOAD(2100 gas).

At least the following ones in HolographOperator: _blockTime, _baseBondAmount, _podMultiplier, _operatorThreshold, _operatorThresholdStep, _operatorThresholdDivisor.

[G-05] Remove blockTimes from OperatorJob.

In HolographOperator, job.blockTimes is always equal to _blockTime so the entry could be removed from the struct. This could be combined with the previous gas optimization for even more savings.

[G-06] Useless check and external call require(!_isContract(holographerAddress), "HOLOGRAPH: already deployed");.

In HolographFactory, at this line there is no need to check if the contract has already been deployed per EIP-684. See here.

Quoting it: "If a contract creation is attempted, due to either a creation transaction or the CREATE (or future CREATE2) opcode, and the destination address already has either nonzero nonce, or nonempty code, then the creation throws immediately, with exactly the same behavior as would arise if the first byte in the init code were an invalid opcode. This applies retroactively starting from genesis."