Infinex 11

When setting vesting entries, the _setVestingEntry function checks that the total allocated amount doesn't exceed the totalReward amount:

if (_amount + campaign.totalAllocated > campaign.totalReward) revert AllocationOverTotalReward();

Reference: RewardCampaign.sol#L359

This check works as intended for new accounts. However, it fails when overriding an existing vesting entry. The issue arises because campaign.totalAllocated includes the old entry's amount, potentially causing the transaction to revert even when the total allocation doesn't actually exceed totalReward.

Consider this scenario: A campaign has a totalReward of 100, with 95 already allocated, including 10 for accountA. If the vestingEntryManager tries to change accountA's allocation to 15, the check fails:

15 + 95 > 100 (true, causing revert)

To fix this, subtract the old amount (= 10) from totalAllocated:

15 + (95 - 10) > 100 (false, allowing the update)

Remediation to Consider

Modify the above check to account for the previously allocated amount when overriding an existing entry.

In order to delete the campaign, the owner must call to deleteCampaign(), which basically sets the campaign.id value to zero:

function deleteCampaign(uint32 _campaignId) external onlyOwner {
    IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
    if (_campaignId == 0 || campaign.id == 0) revert InvalidCampaignId();

    emit CampaignDeleted(_campaignId);

    RewardCampaignStorage._deleteCampaign(_campaignId);

    if (campaign.funded) {
        IERC20(campaign.token).safeTransfer(msg.sender, campaign.totalReward - campaign.totalClaimed);
    }
}

Reference: RewardCampaign.sol#L164-L175

function _deleteCampaign(uint32 _campaignId) internal {
    getStorage().campaigns[_campaignId].id = 0;
}

Reference: RewardCampaignStorage.sol#L110-L112

In the fundCampaign(), startCampaign(), and all the claiming functions don’t restrict whether the campaign is deleted. As a result, a deleted campaign can still be funded, started, and can even be claimed by users when the campaign is deleted

Remediations to Consider

Consider disallowing the call to other functions with a particular campaign if the campaign is deleted

RewardCampaign doesn't properly account for tokens that apply a transfer tax. When such a token is used, the actual amount transferred in the fundCampaign function is less than the specified _amount. This discrepancy leads to an incorrect totalReward calculation, potentially preventing users from claiming rewards as the contract may run out of funds.

Remediation to Consider

To accurately handle fee-on-transfer tokens, calculate the contract's balance before and after the transfer. Use this difference, rather than the specified amount, to update the campaign's total reward.

In the RewardCampaign.claimable() function, the return value will be the amount of token that can be claimed for a particular vesting entry at that moment:

function claimable(uint32 _campaignId, address _account) external view returns (uint256) {
    IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
    IRewardCampaign.VestingEntry memory vestingEntry = RewardCampaignStorage._getVestingEntry(_campaignId, _account);
    if (vestingEntry.amount == 0) return 0;

    uint256 claimableAmount = _claimable(campaign.startDate, campaign.vestingCliff, campaign.vestingDuration, vestingEntry.amount);
    uint256 availableToClaim = claimableAmount - vestingEntry.claimed;
    return availableToClaim;
}

Reference: RewardCampaign.sol#L120-L127

When the campaign is not started, the campaign’s startDate is still 0. Theoretically, the claimable() function should return zero at that time, since no vesting entry can be claimed. But in reality, it will return a maximum amount of token of the vesting entry:

function _claimable(uint256 _start, uint256 _cliff, uint256 _duration, uint256 _amount) internal view returns (uint256) {
    if (block.timestamp < _start + _cliff) {
        return 0;
    } else if (block.timestamp >= _start + _cliff + _duration) {
        return _amount;    //<@@ will go to this case because _start == 0 
    } else {
        return _amount * (block.timestamp - (_start + _cliff)) / _duration;
    }
}

Remediations to Consider

Consider making the claimable() return zero when the campaign has not started yet

function claimable(uint32 _campaignId, address _account) external view returns (uint256) {
    IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
+   if (campaign.startDate == 0) return 0;
    IRewardCampaign.VestingEntry memory vestingEntry = RewardCampaignStorage._getVestingEntry(_campaignId, _account);
    if (vestingEntry.amount == 0) return 0;

    uint256 claimableAmount = _claimable(campaign.startDate, campaign.vestingCliff, campaign.vestingDuration, vestingEntry.amount);
    uint256 availableToClaim = claimableAmount - vestingEntry.claimed;
    return availableToClaim;
}

The _claim() function doesn’t follow check-effect-interaction (CEI) pattern:

function _claim(uint32 _campaignId, address _account, address _destination) internal {
  ...
  IERC20(campaign.token).safeTransfer(_destination, availableToClaim); <@@@ REENTRANCY
  RewardCampaignStorage._setVestingEntryClaimed(_campaignId, _account, availableToClaim);
  RewardCampaignStorage._setCampaignTotalClaimed(_campaignId, availableToClaim);
  ...
}

Reference: RewardCampaign.sol#L330-L346

As a result, the function is vulnerable to reentrancy when the reward token of that campaign supports the ERC777 standard. The attacker can claim many times without changing states, which leads to draining all the reward token that belong to the RewardCampaign contract

The fundCampaign() function is also not following the CEI pattern, which is also vulnerable to reentrancy when the reward token of that campaign supports the ERC777 standard. Even though the impact is not severe, it’s better still to follow the CEI pattern

function fundCampaign(uint32 _campaignId, uint256 _amount) external {
  IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
  if (_ERC2771MsgSender() != campaign.rewarder) revert InvalidRewarder();
  if (campaign.funded) revert CampaignAlreadyFunded();
  if (_amount != campaign.totalReward) revert InvalidAmount();

  emit CampaignFunded(_campaignId, campaign.rewarder, _amount);

  // slither-disable-next-line arbitrary-send-erc20
  IERC20(campaign.token).safeTransferFrom(_ERC2771MsgSender(), address(this), _amount);
  RewardCampaignStorage._setCampaignFunded(_campaignId, true);
}

Reference: RewardCampaign.sol#L182-L193

Remediations to Consider

Consider following CEI pattern in _claim() and fundCampaign() functions

The RewardCampaign contract allows claiming reward tokens to an Infinex account with the account’s sudo signature, using either the claim() function or the claimToAllowlistedWithdrawalAddress() function with an EIP1271 signature. In the claim() function, the hash structure is the combination of _campaignId with uint32 type and _account with address type:

bytes32 requestHash = keccak256(abi.encodePacked(_campaignId, _account));

Reference: RewardCampaign.sol#L299

In claimToAllowlistedWithdrawalAddress() function, the hash structure is the combination of _campaignId with uint32 type, _account with address type, and _withdrawalAddress with address type:

bytes32 requestHash = keccak256(abi.encodePacked(_campaignId, _account, _withdrawalAddress));

Reference: RewardCampaign.sol#L324

The hash structures of these 2 functions are relatively simple. In the future, when more contracts allow the EIP1271 standard for Infinex accounts, it could lead to a single signature being a valid input for different functions from different contracts, potentially leading to unpredictable behavior.

Remediations to Consider

Consider including the RewardCampaign address and corresponding function selector in the hashing structure of the signature

The RewardCampaign contract allows claiming reward tokens to an Infinex account with the account’s sudo signature, using either the claim() function or the claimToAllowlistedWithdrawalAddress() function with an EIP1271 signature. These functions don’t have any mechanism to prevent the same signature from being reused. Even though the reward tokens will be sent to the correct account, it is still a potential griefing vector and we recommend preventing signatures from being replay.

Remediations to Consider

Consider including a nonce mechanism in the hashing structure of the signature

The setVestingEntry function lacks a check to ensure the _amount parameter is greater than zero. Consequently, claiming for an entry with a zero amount will fail, triggering an InvalidVestingEntry() error.

Remediation to Consider

Although this doesn't pose a security risk, it's advisable to prevent the creation of invalid entries in the setVestingEntry function.

In the RewardCampaign.initialize() function, trusted forwarder can be set as address(0)

function initialize(address _owner, address _trustedForwarder) public initializer {
    OwnableUpgradeable.__Ownable_init(_owner);
    ERC2771Context.initialize(_trustedForwarder);
}

Reference: RewardCampaign.sol#L55-L58

This behavior is not harmful because the owner can change trusted forwarder anytime, but since addTrustedForwarder() and removeTrustedForwarder() are not allowing trustedForwarder as address(0), it makes sense to also disallow in the initialize() function for consistency state

Remediations to Consider

Consider disallowing _trustedForwarder to address(0) in the initialize() function

function initialize(address _owner, address _trustedForwarder) public initializer {
    OwnableUpgradeable.__Ownable_init(_owner);

+   if (_trustedForwarder == address(0)) revert Error.NullAddress();
    ERC2771Context.initialize(_trustedForwarder);
}

The id member in the Campaign struct is non-zero when the campaign exists and is set to zero if the campaign is not created or deleted. The id is already stored as a key in the campaigns mapping, hence it’s redundant to have an id member in the Campaign struct.

Consider changing the id member in the Campaign struct to isDeleted member with a boolean type for better clarity and to reduce state redundant

In the RewardCampaignStorage._setVestingEntry function, there’s a mechanism to prevent the underflow error:

if (vestingEntry.amount > _amount) {
 newTotalAllocated = campaign.totalAllocated - (vestingEntry.amount - _amount);
} else {
 newTotalAllocated = campaign.totalAllocated + (_amount - vestingEntry.amount);
}

Reference: RewardCampaignStorage.sol#L160-L164

This logic is redundant due to the following invariant: the totalAllocated of the campaign is equal to the sum of all the vestingEntry amounts of the same campaign. This means the totalAllocated of the campaign is always greater than or equal to any single vestingEntry amount of the same campaign. Therefore, it's impossible for campaign.totalAllocated - vestingEntry.amount to underflow.

Remediations to Consider

Consider removing the underflow mechanism for better simplicity:

function _setVestingEntry(uint32 _campaignId, address _account, uint256 _amount) internal {
    IRewardCampaign.Campaign memory campaign = getStorage().campaigns[_campaignId];
    IRewardCampaign.VestingEntry memory vestingEntry = getStorage().vestingEntries[_campaignId][_account];
    uint256 newTotalAllocated;

-   if (vestingEntry.amount > _amount) {
-       newTotalAllocated = campaign.totalAllocated - (vestingEntry.amount - _amount);
-   } else {
-       newTotalAllocated = campaign.totalAllocated + (_amount - vestingEntry.amount);
-   }

+		newTotalAllocated = campaign.totalAllocated + _amount - vestingEntry.amount;

    getStorage().campaigns[_campaignId].totalAllocated = newTotalAllocated;
    getStorage().vestingEntries[_campaignId][_account] = IRewardCampaign.VestingEntry({ amount: _amount, claimed: 0 });
}

After a campaign has been started, no more vesting entries can be added to the campaign. Thus, there is no need to add more funds to the campaign via fundCampaign.

Remediation to Consider

Consider preventing the addition of funds to campaigns that have already started.

A Rewarder and VestingManager address can be set via setCampaignRewarder and setCampaignVestingEntryManager for campaigns. Ideally, this should only be possible for campaigns that haven't started yet. However, the current implementation allows setting or changing these addresses even for ongoing campaigns.

Remediation to Consider

Restrict the ability to set Rewarder and VestingManager addresses to campaigns that have not yet begun.

In the ERC2771BaseUpgradeable abstract contract, there are addTrustedForwarder() and removeTrustedForwarder() functions that the child contract should override. However, because these functions contain empty braces { }, the compiler treats them as implemented with empty logic. As a result, the compiler will not throw any error when a child contract does not override the addTrustedForwarder() and removeTrustedForwarder() functions from the ERC2771BaseUpgradeable contract. Child contracts that forget to override these functions may not be able to add or remove trusted forwarders.

function addTrustedForwarder(address _trustedForwarder) external virtual returns (bool) { }
function removeTrustedForwarder(address _trustedForwarder) external virtual returns (bool) { }

Reference: ERC2771BaseUpgradeable.sol#L54-L61

Remediations to Consider

Consider changing the code as follows to make function overriding mandatory for child contracts:

-    function addTrustedForwarder(address _trustedForwarder) external virtual returns (bool) { }
+    function addTrustedForwarder(address _trustedForwarder) external virtual returns (bool);

-    function removeTrustedForwarder(address _trustedForwarder) external virtual returns (bool) { }
+    function removeTrustedForwarder(address _trustedForwarder) external virtual returns (bool);

In the RewardCampaign contract, the rewarder of the campaign is set initially in the createCampaign() function by the owner, and can be changed later with the setCampaignRewarder() function by the owner. Only the rewarder of the campaign can fund the corresponding campaign via the fundCampaign() function, which is too strict.

Remediation to Consider

Consider removing the rewarder mechanism and making fundCampaign() function permissionless for more flexibility.

According to the Blast’s documentation, the Blast Point operator must be an EOA:

An operator is an EOA (externally owned account) whose private key is accessible to an internet-connected server.

However, in the BlastPointDistributor contract, there's no check to verify whether the operator is an EOA or not. Consider adding a validation to restrict the operator to EOA addresses only.

In the setVestingEntries() function, _setVestingEntry() internal function is called multiple times in the loop with the same _campaignId argument:

function setVestingEntries(uint32 _campaignId, address[] calldata _accounts, uint256[] calldata _amounts)
    external
    onlyVestingEntryManager
{
    if (_accounts.length != _amounts.length) revert Error.InvalidLength();
    for (uint256 i = 0; i < _accounts.length; i++) {
        _setVestingEntry(_campaignId, _accounts[i], _amounts[i]);
    }
}

Reference: RewardCampaign.sol#L228-L236

In the _setVestingEntry() function, the _campaignId parameter is restricted by these two requirements:

function _setVestingEntry(uint32 _campaignId, address _account, uint256 _amount) internal {
    IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);

    if (_campaignId == 0 || campaign.id == 0) revert InvalidCampaignId();
    if (campaign.startDate > 0) revert CampaignHasStarted();

    ...
}

Reference: RewardCampaign.sol#L313-L322

While calling _setVestingEntry() multiple times through the setVestingEntries() function, the same two requirements are checked, which is redundant and should be only checked once throughout the setVestingEntries() function

Remediations to Consider

Consider moving these two requirements from _setVestingEntry() function to the higher functions, which is setVestingEntry() and setVestingEntries() functions

function setVestingEntry(uint32 _campaignId, address _account, uint256 _amount) external onlyVestingEntryManager {
+   IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
+   if (_campaignId == 0 || campaign.id == 0) revert InvalidCampaignId();
+   if (campaign.startDate > 0) revert CampaignHasStarted();

    _setVestingEntry(_campaignId, _account, _amount);
}

function setVestingEntries(uint32 _campaignId, address[] calldata _accounts, uint256[] calldata _amounts)
    external
    onlyVestingEntryManager
{
    if (_accounts.length != _amounts.length) revert Error.InvalidLength();

+   IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);
+   if (_campaignId == 0 || campaign.id == 0) revert InvalidCampaignId();
+   if (campaign.startDate > 0) revert CampaignHasStarted();

    for (uint256 i = 0; i < _accounts.length; i++) {
        _setVestingEntry(_campaignId, _accounts[i], _amounts[i]);
    }
}

function _setVestingEntry(uint32 _campaignId, address _account, uint256 _amount) internal {
    IRewardCampaign.Campaign memory campaign = RewardCampaignStorage._getCampaign(_campaignId);

-   if (_campaignId == 0 || campaign.id == 0) revert InvalidCampaignId();
-   if (campaign.startDate > 0) revert CampaignHasStarted();

    ...
}