hats-finance / VMEX-0xb6861bdeb368a1bf628fc36a36cec62d04fb6a77

LP token lending protocol
MIT License
2 stars 4 forks source link

DistributionManager.configureRewards does not prevent the insertion of duplicate values in state variable arrays #10

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @aviggiano Submission hash (on-chain): 0x0cad7026c3bdd92bacc53a263f1df1e28c41e90ed6947dac72dc39458ecbb0bf Severity: low

Description: Description\

In DistributionManager.sol:43, DistributionManager.configureRewards does not validate that incentivizedAsset is not already present in _allIncentivizedAssets array. For example, if the EMISSION_MANAGER sends duplicate elements by mistake, these will be included into the array, which could disrupt the system functioning as intended.

Attack Scenario\

A EMISSION_MANAGER may deliberately or mistakenly introduce duplicates of incentivizedAsset to the _allIncentivizedAssets array. This could cause disruption to the reward distribution process by leading to data inconsistency or inaccuracies in reward distribution.

Attachments

  1. Proof of Concept (PoC) File
function configureRewards(RewardConfig[] calldata config) external override {
  require(msg.sender == EMISSION_MANAGER, 'ONLY_EMISSION_MANAGER');
  uint256 length = config.length;
  for (uint256 i; i < length;) {
    DistributionTypes.IncentivizedAsset storage incentivizedAsset = _incentivizedAssets[
      config[i].incentivizedAsset
    ];
    DistributionTypes.Reward storage reward = incentivizedAsset.rewardData[config[i].reward];

    // @audit-issue DistributionManager.configureRewards does not validate that incentivizedAsset is not present in _allIncentivizedAssets array. For example, if the EMISSION_MANAGER sends duplicate elements by mistake, these will be included into the array, disrupting the system.
    if (incentivizedAsset.numRewards == 0) {
      // this incentivized asset has not been introduced yet
      _allIncentivizedAssets.push(config[i].incentivizedAsset);
      incentivizedAsset.decimals = IERC20Detailed(config[i].incentivizedAsset).decimals();
    }
    if (reward.lastUpdateTimestamp == 0) {
      // this reward has not been introduced yet
      incentivizedAsset.rewardList[incentivizedAsset.numRewards] = config[i].reward;
      incentivizedAsset.numRewards++;
      _allRewards.push(config[i].reward);
    }
  1. Remediation

Validate that the config array does not contain any duplicates.

ksyao2002 commented 1 year ago

Hi, thanks for the report on how duplicate elements could be inserted into the _allIncentivizedAssets array. I was wondering under what circumstance could duplicates enter? Since there is the line

if (incentivizedAsset.numRewards == 0) {

guarding the insertion to the array. Even if the config had multiple instances of one .incentivizedAsset, it would update the numRewards for that asset, and would skip it in the next iteration.

Please let me know if I'm missing a case where this can happen.

aviggiano commented 1 year ago

Hello @ksyao2002,

I'm sorry for not getting back to you sooner, I was double checking this was indeed a potential issue. Also, I apologize for not being clearer in the previous report.

The problem is not in line if (incentivizedAsset.numRewards == 0) { but instead in if (reward.lastUpdateTimestamp == 0) {.

If the config contains duplicate elements, the check for reward.lastUpdateTimestamp will return 0, and the variable incentivizedAsset.numRewards will be incremented twice. Also, _allRewards will now contain duplicate config[].reward elements.

ksyao2002 commented 1 year ago

Thanks for the reply. If config has a duplicate reward, that means incentivizedAsset.rewardData[config[i].reward] would return the same reward storage variable that was set in a previous iteration, meaning that the reward.lastUpdateTimestamp has already been set, and won't be zero. I'm still not fully understanding under what circumstance this can be a vulnerability.

Maybe it'll help if you can provide a POC?

aviggiano commented 1 year ago

Hey Sorry for not getting back promptly. I wanted to write a PoC before responding, but unfortunately, I haven't had the time. I believe you can just close this issue, if I find anything relevant I'll submit a report on your bug bounty program! Sorry for any inconvenience.