6.1 Duplicate entries in _modifyIncentiveStreams(...) result in loss of incentives
The IncentiveLocker::createIncentiveCampaign(...) function is used to create a campaign which invokes UmaMerkleChefAV::processIncentiveCampaignCreation(...). The processIncentiveCampaignCreation(...) invokes _modifyIncentiveStreams(...) to update the current rate for each incentive in incentiveCampaignIdToIncentiveToCurrentRate mapping.
function processIncentiveCampaignCreation(
bytes32 _incentiveCampaignId,
address[] memory _incentivesOffered,
uint256[] memory _incentiveAmountsOffered,
bytes memory _actionParams,
address /*_ip*/
)
external
override
onlyIncentiveLocker
{
ActionParams memory params = abi.decode(_actionParams, (ActionParams));
uint32 startTimestamp = params.startTimestamp;
uint32 endTimestamp = params.endTimestamp;
// ...
// Apply the modification to streams
_modifyIncentiveStreams(Modifications.INIT_STREAMS, _incentiveCampaignId, startTimestamp, endTimestamp, _incentivesOffered, _incentiveAmountsOffered);
}
function _modifyIncentiveStreams(
Modifications _modification,
bytes32 _incentiveCampaignId,
uint32 _startTimestamp,
uint32 _endTimestamp,
address[] memory _incentives,
uint256[] memory _incentiveAmounts
)
internal
{
// ...
// Make modifications to the appropriate incentive streams
uint256 numIncentives = _incentives.length;
uint256[] memory updatedRates = new uint256[](numIncentives);
for (uint256 i = 0; i < numIncentives; ++i) {
// If creating a new campaign
if (_modification == Modifications.INIT_STREAMS) {
// Initialize the rate on creation
updatedRates[i] = (_incentiveAmounts[i]).divWadDown(_endTimestamp - _startTimestamp);
// If adding or removing incentives from an already created campaign
} else {
// ...
}
// Update the rate for this incentive to the current rate
// @audit If _incentives includes duplicate entries, it will override the rate corresponding to previous entry.
incentiveCampaignIdToIncentiveToCurrentRate[_incentiveCampaignId][_incentives[i]] = updatedRates[i];
}
// ...
}
However, if _incentives in _modifyIncentiveStreams(...) includes duplicate entries, it will override the rate corresponding to the previous entry. This would result in incorrect accounting.
Although, the merkle root submitted by IP can still correspond to correct accounting.
Recommendation
Consider handling the case where incentives array while creating a campaign includes duplicate entries.
6.2 Malicious incentive token can restrict claiming of other incentives
The claimIncentives(...) function is responsible for claiming incentives from a specific incentive campaign. It uses _remitIncentivesAndFees(...) to process individual incentive claims by iterating over each offered incentive. To transfer incentive amount and account for protocol fee, it calls _pushIncentivesAndAccountFees(...).
function _remitIncentivesAndFees(
ICS storage _ics,
address _ap,
address _protocolFeeClaimant,
address[] memory _incentives,
uint256[] memory _incentiveAmountsOwed
)
internal
returns (uint256[] memory incentiveAmountsPaid, uint256[] memory protocolFeesPaid)
{
// Cache for gas op
uint256 numIncentives = _incentives.length;
// ...
// Initialize array for event emission
incentiveAmountsPaid = new uint256[](numIncentives);
protocolFeesPaid = new uint256[](numIncentives);
for (uint256 i = 0; i < numIncentives; ++i) {
// Cache for gas op
address incentive = _incentives[i];
uint256 incentiveAmountOwed = _incentiveAmountsOwed[i];
// Account for spent incentives to prevent co-mingling of incentives
_ics.incentiveToAmountRemaining[incentive] -= incentiveAmountOwed;
// Calculate fee amounts based on the claim ratio.
protocolFeesPaid[i] = incentiveAmountOwed.mulWadDown(_ics.protocolFee);
// Calculate the net incentive amount to be paid after applying fees.
incentiveAmountsPaid[i] = incentiveAmountOwed - protocolFeesPaid[i];
// Push incentives to the action provider and account for fees.
// @audit If this reverts for one incentive, then the transfer for all other incentives will also revert.
_pushIncentivesAndAccountFees(incentive, _ap, _protocolFeeClaimant, incentiveAmountsPaid[i], protocolFeesPaid[i]);
}
}
function _pushIncentivesAndAccountFees(
address incentive,
address ap,
address protocolFeeClaimant,
uint256 incentiveAmount,
uint256 protocolFeeAmount
)
internal
{
// Take fees and push incentives to action provider
if (isPointsProgram(incentive)) {
// ...
} else {
// Make fees claimable by fee claimant
feeClaimantToTokenToAmount[protocolFeeClaimant][incentive] += protocolFeeAmount;
// Transfer incentives to the action provider
ERC20(incentive).safeTransfer(ap, incentiveAmount);
}
}
However, If _pushIncentivesAndAccountFees(...) reverts for one incentive, then all other incentives can't be transferred too. This exploit allows IP or Co-IP to add a malicious token as incentive which behaves maliciously after the merkle root is verified, restricting claiming of incentives.
Although, a whitelisted asserter can then add a merkle root such that it excludes the malicious token.
Recommendation
Consider updating the implementation for transferring the incentives to handle the revert case.
Resolution
The asserters would just post another merkle root with the malicious tokens excluded from the leaves.
6.3 UmaMerkleOracleBase::blacklistAsserters(...) does not revoke assertion privileges
The blacklistAsserters(...) function in UmaMerkleOracleBase is intended to revoke assertion privileges by blacklisting asserters. However, the current implementation mistakenly sets the asserterToIsWhitelisted mapping to true for the provided addresses, which has the opposite effect—whitelisting the asserters instead of blacklisting them:
function blacklistAsserters(address[] memory _blacklistedAsserters) public virtual onlyOwner {
uint256 numAsserters = _blacklistedAsserters.length;
for (uint256 i = 0; i < numAsserters; ++i) {
==> asserterToIsWhitelisted[_blacklistedAsserters[i]] = true;
}
emit AssertersBlacklisted(_blacklistedAsserters);
}
Though malicious asserters attempting to assert an incorrect Merkle root will lose the bond amount since the oracle will dispute their assertions.
Recommendation
Consider setting the asserterToIsWhitelisted to false instead of true.
