Distribution of tokens in recordKeyPurchase

[code423n4]

Handle

pauliax

Vulnerability details

Impact

function recordKeyPurchas does not distribute the tokens on chainId > 1 when the balance is not sufficient:

  uint balance = IMintableERC20(udt).balanceOf(address(this));
  if (balance > tokensToDistribute) {
    // Only distribute if there are enough tokens
    IMintableERC20(udt).transfer(_referrer, tokensToDistribute - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  }

I see at least 2 problems here. First, the check should be inclusive >=, because now when balance = tokensToDistribute, it will still skip the distribution.
Another problem, I think it is not fair when only a small fraction is missing, you should consider distributing the whole balance in this case.

Recommended Mitigation Steps

Here is my prosposed solution:

  uint balance = IMintableERC20(udt).balanceOf(address(this));
  if (balance >= tokensToDistribute) {
    IMintableERC20(udt).transfer(_referrer, tokensToDistribute - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  } else {
    devReward = balance * 20 / 100;
    IMintableERC20(udt).transfer(_referrer, balance - devReward);
    IMintableERC20(udt).transfer(owner(), devReward);
  }

[julien51]

I see at least 2 problems here. First, the check should be inclusive >=, because now when balance = tokensToDistribute, it will still skip the distribution.

It is a good point, but also almost impossible to achieve = because :

1. If the number of tokens to be distributed is based on gas, the gas price + token price would have to be exactly aligned with what is left on the contract (which seems practically impossible given the precision!)
2. if the number of tokens to be distributed is based on actual GDP contribution, the only way to make that true is to have a "Infinite" contribution which is itself impossible.

[julien51]

Another problem, I think it is not fair when only a small fraction is missing, you should consider distributing the whole balance in this case.

I am not sure I understand this comment. What is "missing"?

[0xleastwood]

I think the warden has provided useful information, however, it is more in line with a low severity issue.

1 — Low (L): vulns that have a risk of 1 are considered “Low” severity when assets are not at risk. Includes state handling, function incorrect as to spec, and issues with comments.