Sparkn

Summary

Winners array lacks duplicates check which invites same people participating under different aliases or organizer making mistakes and duplicating some of the winners while missing other legitimate winners.

Vulnerability Details

Given that the winners array lacks any type of verification with respect to the elements contained (other than length != 0) the function could send multiple payments to the same address, which in most cases is a mistake of the organizer or a malicious participant.

Impact

Double payments of rewards to some people while other people completely miss their prizes.

Tools Used

Manual review

Recommendations

There are 2 ways of making this check:

1. Organizer sorts the list beforehand (or gets it sorted from the frontend of the protocol) and the following adaptations are made:


// if there is no token to distribute, then revert
if (totalAmount == 0) revert Distributor__NoTokenToDistribute();
​
++ address previousWinner; //Track the last distributed winner
​
uint256 winnersLength = winners.length; // cache length
for (uint256 i; i < winnersLength;) {
​
++ // Skip duplicate winners
++ if (winners[i] == previousWinner) {
++ continue;
++ }
​
uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
erc20.safeTransfer(winners[i], amount);
unchecked {
++i;
}
​
++ previousWinner = winners[i];
}

The other option in this case is to modify the check to revert the transaction:

++error Distributor__DuplicateWinner();
​
...
​
++ // Skip duplicate winners
++ if (winners[i] == previousWinner) {
++ revert Distributor__DuplicateWinner();
++ }
2. If the array is not sorted before hand we could employ a sorting algorithm like Quicksort or Mergesort to sort it in Solidity, but keep in mind that it might be gas-intensive and may not be practical for large arrays due to gas costs and block gas limits.