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.