Holograph contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L439-L440

Vulnerability details

Impact

The parameter tokenAddress of _payoutToken/_payoutTokens can be any token. According to https://github.com/d-xo/weird-erc20/#missing-return-values, some tokens have no return value when transferred, so in the _payoutToken/_payoutTokens functions, even if the transfer is successful, since there is no return value, the return value will be considered false, causing the function to not work, thereby locking these tokens in the contract

Proof of Concept

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L416-L417 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L439-L440

Tools Used

None

Recommended Mitigation Steps

Using safeTransfer

    function safeTransfer(address token, address to, uint256 value) internal {
        require(token.code.length > 0);
        (bool success, bytes memory data) =
        token.call(abi.encodeWithSelector(IERC20.transfer.selector, to, value));
        require(success && (data.length == 0 || abi.decode(data, (bool))));
    }

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L415-L416

Vulnerability details

Impact

The parameter tokenAddress of _payoutToken/_payoutTokens can be any token. Some tokens (e.g. LEND) revert when transfering a zero value amount. https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

PA1D._payoutToken/_payoutTokens might revert when sending = 0 and tokenAddress is a revert on zero value transfers token.

sending may be 0 due to loss of precision.

        sending = ((bps[i] * balance) / 10000);
        require(erc20.transfer(addresses[i], sending), "PA1D: Couldn't transfer token");

This will cause the function not to work, thereby locking these tokens in the contract

Proof of Concept

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L415-L416 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L438-L439

Tools Used

None

Recommended Mitigation Steps

Require sending > 0 before sending tokens

Lines of code

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

Vulnerability details

Impact

In the executeJob function, when the job has not been executed by the selectedOperator and fallbackOperator for a period of time, anyone can call the executeJob function to execute the job and get the utilityToken reward (the utilityToken reward is stored in the _ bondedAmounts variable).

        _bondedAmounts[job.operator] -= amount;
        /**
         * @dev the slashed amount is sent to current operator
         */
        _bondedAmounts[msg.sender] += amount;

However, if the user is not a bonded operator, the user cannot call the unbondUtilityToken function to withdraw the utilityToken because _bondedOperators is 0

  function unbondUtilityToken(address operator, address recipient) external {
    /**
     * @dev validate that operator is currently bonded
     */
    require(_bondedOperators[operator] != 0, "HOLOGRAPH: operator not bonded");

And since _bondedAmounts is not 0, the user will not be able to call bondUtilityToken to set the _bondedOperators.

  function bondUtilityToken(
    address operator,
    uint256 amount,
    uint256 pod
  ) external {
    /**
     * @dev an operator can only bond to one pod at any give time per network
     */
    require(_bondedOperators[operator] == 0 && _bondedAmounts[operator] == 0, "HOLOGRAPH: operator is bonded");
    unchecked {

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider sending the utilityToken directly to the user in the executeJob function if the caller is not a bonded operator

        _bondedAmounts[job.operator] -= amount;
        /**
         * @dev the slashed amount is sent to current operator
         */
       if(_bondedOperators[msg.sender] == 0)  _utilityToken().transfer(msg.sender, amount);
       else  _bondedAmounts[msg.sender] += amount;

Or only allow the operator to call the executeJob function

  function executeJob(bytes calldata bridgeInRequestPayload) external payable {
        require(_bondedOperators[msg.sender] != 0)

[Low-01] gasleft() should be greater than gasLimit + a constant

In the executeJob function, gasLimit represents the gas limit required to execute cross-chain messages, while in the executeJob function, gasleft() has some other gas consumption (such as setting _failedJobs and _inboundMessageCounter) in addition to executing cross-chain messages. So gasleft() should be greater than gasLimit + a constant

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

[Low-02] Attacker can block LayerZero channel

According to the LayerZero docs, the default behavior is that when a transaction on the destination application fails, the channel between the src and dst app is blocked. Before any new transactions can be executed, the failed transaction has to be retried until it succeeds.

See https://layerzero.gitbook.io/docs/faq/messaging-properties#message-ordering & https://layerzero.gitbook.io/docs/guides/advanced/nonblockinglzapp

So an attacker is able to initiate a transaction they know will fail to block the channel.

An example implementation of the non-blocking approach by LayerZero: https://github.com/LayerZero-Labs/solidity-examples/blob/main/contracts/lzApp/NonblockingLzApp.sol

[Low-03] owner can rug all assets in PA1D

The owner can set itself as a unique payoutAddresses in the configurePayouts function, and rug all assets in the contract through the getEthPayout/getTokenPayout/getTokensPayout functions

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L471-L480 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/enforcer/PA1D.sol#L497-L520

##[Low-04] Not compatible with Rebasing/Deflationary/Inflationary tokens In the HolographOperator contract, UtilityTokens are not supported as rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. This can lead to failed withdrawals or lost assets due to balances different from _bondedAmounts

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L839-L840 https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L889-L890