Immunefi Arbitration Protocol

When a Vault wants to withdraw its funds, it has to go through the WithdrawalSystem which is in turn walled behind a Timelock. So, the intended behaviour is to not allow for instant withdrawals at any point or any state of the system or the Vault. However, a mechanism that allows a Vault to bypass this process exists by using the RewardSystem and there are two cases where this situation can be exploited by the Vault:

• The RewardSystem defines the sendRewardByVault function that is expected to be used by the Vaults to reward the whitehats for a valid submission. This can be exploited by the Vaults by creating a fake issue submission by a Vault-controlled “whitehat” and then issuing a reward for this submission summing up to the total holdings of the Vault, which effectively performs a covert withdrawal.

• This situation can be further exploited in the scenario where a real whitehat is about to submit an arbitration for that Vault. The Vault could frontrun the arbitration transaction and use this alternative way to immediately withdraw their funds before the arbitration starts, leaving the Vault empty and the upcoming arbitration unable to be fulfilled in the future.

A possible solution would be to set a minimal timelock to that function too, so that to delay the reward distribution slightly and prevent the frontrunning scenario by checking the arbitration status on execution of the actual sendReward operation.

In the RewardTimelock contract, the queued transactions are set to expire based on the txExpiration global variable. More specifically, if the elapsed time from their queueing exceeds the txExpiration value, then these transactions can not be executed anymore. However, there is no update in their state to indicate that, which means that even if they have expired they maintain their TxState.Queued state. It is not either possible to cancel an expired transaction and set its state to TxState.Canceled.

To execute a transaction, the following checks need to succeed:

• The transaction’s state should be TxState.Queued

• The transaction should have matured, which means to have waited txCooldown time from the initial submission

• The transaction should not have expired, which means that the current timestamp should not be outside the allowed time window for execution OR the txExpiration should be 0, which means that there would not be an expiration for any queued transaction

As a result, if the txExpiration value becomes 0 in the future, for any reason, all the previously expired transactions would be again resumed and able to be executed.

This could potentially pose security issues in this situation in case some of the previously expired transactions could disrupt the proper execution of the protocol or any assumptions were made by any entity based on their expired status.

Similarly, transactions queued when the txExpiration was 0 would lose this indefinite ability for execution if the variable changes back to a non-zero value rendering them immediately expired if the new value is smaller than the already elapsed time for these transactions.

It is worth mentioning though that the similar Timelock contract does not suffer from that issue as each queued transaction stores its own expiration and cooldown data separate from the global variables, hence being resilient in such changes of the global parameters.

function queueTransaction(
    address to,
    uint256 value,
    bytes calldata data,
    Enum.Operation operation,
    address vault,
    uint256 cooldown,
    uint256 expiration
) external onlyRole(QUEUER_ROLE) {
    ...
    // Dedaub: The cooldown and expiration arguments are the
    //         at-the-time-of-submission global variables provided by the
    //         WithrawalSystem and they are stored locally which makes
    //         them resistant to future changes of the global variables
    txHashData[txHash].queueTimestamp = uint40(block.timestamp);
    txHashData[txHash].cooldown = uint32(cooldown);
    txHashData[txHash].expiration = uint32(expiration);
    txHashData[txHash].state = TxState.Queued;
    ...
}

We suggest implementing a similar mechanism to the RewardTimelock as well to be sure that no expired transactions would ever be able to be executed in the future and make them resistant to this kind of change, but with that solution, you should also consider the case where transactions with indefinite execution allowance would keep the indefinite allowance even when the txExpiration changes which might not be desired in general. So, the solution should special case this category to account for this if it does not follow the desired behavior.

In the RewardTimelock, a request to send rewards to an address can be queued for a Vault only if that Vault is currently in arbitration mode. However, the queued transaction is not tied up with the specific arbitrationId that allowed the queuing to happen. This allows a queued transaction to be processed even when the initial arbitration has been closed and a new one has been opened. Consider the following scenario for example:

• An arbitration with ID 1 is active for a Vault

• The Vault queues a transaction using the RewardTimelock which is allowed due to the active arbitration

• The arbitration with ID 1 is then closed before the queued transaction matures

• Some time elapses and the transaction matures and can be executed but there is no active arbitration to allow this operation go through

• A new arbitration with ID 2 is then initiated by a whitehat

• The matured and pending transaction can then be immediately executed as the Vault is now in arbitration mode again

This behaviour deviates from the assumption taken by the protocol that when an arbitration is activated, the Vault would not be able to send any rewards until txCooldown period elapses, which is not true for the scenario described above.

A possible solution would be to connect the transaction with the specific arbitrationId that allowed its queueing to ensure that if the arbitration closes all the pending transactions get immediately invalidated for future use.

The setVaultFee function of the VaultFees contract allows the feeRecipient of a vault to be set to address(0) while the feeBps of a vault is set to a particular value.

On the other hand, the getFee function of the VaultFees contract checks whether the feeRecipient of a vault is address(0), and in that case, overwrites both the feeRecipient and the feeBps values of the vault with default values, losing the set feeBps value in the process.

When the system is in emergency mode all the Vaults are locked and can not make any calls through the ImmunefiModule. As a result, a Vault would not be possible to initiate an arbitration when in emergency as the requestArbVault would revert on the fee payment, which goes through ImmunefiModule.

However, this is not the case for the requestArbWhitehat function. Presently, there is no check for the emergency status of the system and this allows any whitehat to put a Vault in arbitration while the system is halted.

There do not seem to be any concerning consequences, in particular, for this inconsistency, but it could lead to unintended results in a future update.

In the RewardTimelock contract the Vaults can queue reward distribution transactions when they are in arbitration. However, if the rewards are split among a large number of tokens, then the execution of the transaction might run out of gas and not be able to be executed.

The RewardTimelock::executeRewardTransaction function calculates the value in dollars of the tokens sent for distribution. It iterates over all of them, fetches their prices from Chainlink or other custom oracles and if the sum of the supplied tokens is inside the allowed dollar amount, taking slippage into account, then it continues with the actual execution of the reward distribution through the ImmunefiModule. This in turn iterates over all the tokens again to send them to the recipient and also sends a fee to Immunefi for processing the operation for each token.

All this indicates that there are several gas consuming operations happening over the unbounded number of tokens given for execution. Of course, this is user controlled and the caller can adjust the provided tokens so that the transaction does not revert, but if the required dollar amount can not be fulfilled by a smaller number of tokens than the one that runs out of gas, then the Vault would not be able to process the reward distribution. This could happen if the Vault has sent several tokens to the Vault that sum up to the final bounty. This is of course related to the state where an arbitration is enabled, as when it is not, the Vaults can use the RewardSystem::sendRewardByVault function multiple times to distribute the rewards to the whitehats.

A possible mitigation could be to only allow a bounded number of tokens to be used as possible reward tokens by a Vault.

The ImmuneFi team is able to upgrade most of the contracts of the protocol, arbitrarily changing their logic, and is able to give the ARBITER_ROLE and EXECUTOR_ROLE to various addresses. The arbiter has wide discretion on how to distribute the funds inside a vault once a claim is filed by a whitehat, and addresses with the EXECUTOR_ROLE are able to execute transactions directly on the vault, potentially draining it of funds. The ImmuneFi team have recognised and disclosed these aspects of the protocol to the auditing team and acknowledged that projects using the protocol need to trust ImmuneFi not to abuse these powers. Furthermore, it was stated that the ARBITRER_ROLE should be granted to a highly trusted 3rd-party entity.

In the Arbitration contract, the function requestArbWhitehat is meant to be used by a whitehat (or on behalf of them) to initiate an arbitration process on a specific Vault. The protocol defines that an Arbitration can be opened only by two ways:

• Either by the Vault itself, by calling requestArbVault

• Or by a whitehat, by calling requestArbWhitehat

In both cases, the caller should have to pay a fee (in the order of $10K) to the feeRecipient which is assumed to be a trusted entity. However, the feeRecipient has the ability to set a Vault in arbitration at no cost, by calling the requestArbWhitehat function. The steps to accomplish it are as follows:

• The feeRecipient calls the requestArbWhitehat function providing the target vault as an argument

• For the whitehat argument they provide a specially crafted contract that trivially accepts all ERC1271 signatures which effectively bypasses the check of the SignatureCheckerUpgradeable.isValidSignatureNow function

• Finally, _feeAmount tokens are self transferred and the execution completes successfully, with the targeted vault having registered a new (invalid) arbitrationId which immediately puts the Vault in arbitration mode

function requestArbWhitehat(
    uint96 referenceId,
    address vault,
    address whitehat,
    bytes calldata signature
) external {
    bytes32 arbitrationId =
      computeArbitrationId(referenceId, vault, whitehat);
    ...
    // Dedaub: The feeRecipient can register a new arbitration at no cost
    vaultsOngoingArbitrations[vault].add(arbitrationId);
    ...
    require(
        // Dedaub: This check can be bypassed by providing as the whitehat
        //         a specially crafted contract that would accept any
        //         ERC1271 signature
        SignatureCheckerUpgradeable.isValidSignatureNow(
          whitehat, inputHash, signature),
        "Arbitration: invalid request arbitration by whitehat signature"
    );
    ...
    if (_feeAmount > 0) {
        // Dedaub: Self-transfer for the feeRecipient
        feeToken.safeTransferFrom(msg.sender, feeRecipient, _feeAmount);
    }
}

As mentioned, the feeRecipient is considered to be trusted, but we bring this up for visibility and because it also deviates from the assumptions taken on how the protocol is expected to operate.

In the Timelock contract, the canExecuteTransaction function returns whether a txHash can be executed or not. However, this function does not check all the parameters that are checked by the actual execution function resulting in misleading answers. More specifically, the function does not check whether the Vault is frozen or not. It only checks if the timestamps are in the valid time window that would allow the transaction to execute.

function canExecuteTransaction(
    bytes32 txHash
) public view returns (bool) {
    TxStorageData memory txData = txHashData[txHash];
    // Dedaub: Missing check to ensure that the Vault is not frozen
    return
        txData.state == TxState.Queued &&
        txData.queueTimestamp + txData.cooldown <= block.timestamp &&
        (txData.expiration == 0 || txData.queueTimestamp + txData.cooldown
          + txData.expiration > block.timestamp);
}

The protocol uses the Safe contracts at v1.3.0. However, in the later versions, an additional check was added to the code of the checkNSignatures function when the signature is a contract signature (ref. Safe.sol at v1.4.0).

require(keccak256(data) == dataHash, "GS027");

We report this here for visibility and possible examination of the differences between the used and the available versions to ensure that all the fixes of future versions have been taken under consideration.

In the RewardTimelock contract, there are several places where some gas optimisations are possible by caching the used state variables. For example:

• In the executeRewardTransaction, cancelTransaction and canExecuteTransaction functions, there are several checks to ensure that the cooldown and the expiration periods are being respected for a specific txHash. However, the statements use the txCooldown and txExpiration storage variables twice during that process. You could cache these values to save the unnecessary SLOADs (2 for the executeRewardTransaction and the canExecuteTransaction and 1 for the cancelTransaction).

function executeRewardTransaction(
    bytes32 txHash,
    ...
) external {
    ...
    // Dedaub: You could cache both txCooldown and txExpiration
    //         to save 2 SLOADs
    require(
        txData.queueTimestamp + txCooldown <= block.timestamp,
        "RewardTimelock: transaction is not yet executable"
    );
    require(
        txExpiration == 0 || txData.queueTimestamp + txCooldown +
          txExpiration > block.timestamp,
        "RewardTimelock: transaction is expired"
    );
    ...
}

function cancelTransaction(
    bytes32 txHash
) external {
    ...
    // Dedaub: You could cache txExpiration to save 1 SLOAD
    require(
        txExpiration == 0 || txData.queueTimestamp + txCooldown +
          txExpiration > block.timestamp,
        "RewardTimelock: transaction is expired"
    );
    ...
}

function canExecuteTransaction(
    bytes32 txHash
) external view returns (bool) {
    ...
    // Dedaub: You could cache both txCooldown and txExpiration
    //         to save 2 SLOADs
    return
        txData.state == TxState.Queued &&
        txData.queueTimestamp + txCooldown <= block.timestamp &&
        (txExpiration == 0 || txData.queueTimestamp + txCooldown +
          txExpiration > block.timestamp);
}

In the ScopeGuard and in AccessControlGuardable contracts, there are several places where gas optimisations are possible by using the function arguments to emit the events instead of the state variables. For example:

• All the setter functions of the ScopeGuard (setTargetAllowed, …, setAllowedFunction), simply update the values of the storage variables and emit an event related to this operation. However, the event is emitted by fetching the newly updated value from storage when the function argument could be used instead to save that unnecessary SLOAD.

function setTargetAllowed(
    address target,
    bool allow
) public onlyOwner {
    // Dedaub: You could use the argument allow to emit the event
    //         instead of retrieving the recently updated with the
    //         same value storage variable
    allowedTargets[target].allowed = allow;
    emit SetTargetAllowed(target, allowedTargets[target].allowed);
}

• The setGuard of the AccessControlGuardable also emits the event using the recently updated storage variable instead of using the provided argument.

function setGuard(
    address _guard
) external onlyRole(DEFAULT_ADMIN_ROLE) {
    ...
    // Dedaub: Use the _guard variable to save an SLOAD
    guard = _guard;
    emit ChangedGuard(guard);
}

The _getVaultTxsPaginatedReversed function of the RewardTimelockBase contract has a for loop which counts down by initially setting the loop index i to txHashes.length - start - 1. However if start >= txHashes.length, the loop will revert because variable i is of type uint256. It may make sense to require that start < txHashes.length as an additional validation in order to avoid this scenario and return an indicative error message instead.

The WithdrawalSystem contract has two functions setTxCooldown and _setTxCooldown which take a cooldown parameter of type uint256, which is eventually cast to uint32 when either of the functions are run.

This is inconsistent with the setTxCooldown and _setTxCooldown functions of the RewardTimelockBase contract, which take a uint32 as the newTxCooldown parameter and avoid this cast.

One of these representations should be chosen unless there is a good reason for this divergence.

In the WithdrawalSystemBase contract, the _setTxCooldown function directly sets the given value to the txCooldown variable without requiring it to be greater than 0. In contrast, the RewardTimelockBase similar setter has such a check ensuring that the new txCooldown is > 0.

In the ArbitrationBase contract, the _setFeeAmount function has a commented out require statement that if enabled would prevent the feeAmount to be set to 0. We bring this up for visibility in case it was left commented out by mistake and the desired behavior would be to have non-zero feeAmounts.

There are a few places where there are a few typos in the comments:

• AccessControlBaseModule:39

// Zero out the redundant transaction information only used for Safe multisig trans[a]ctions.

• AccessControlGuardable:18

  // Dedaub: It says guard_ when the variable used is named _guard
  // `guard_` does not implement IERC165.
  
• ScopeGuard:75

  @notice Sets whether or not a target can be sent to
  (incluc[d]es fallback/receive functions).
  

The code is compiled with Solidity 0.8.18. Version 0.8.18, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.