Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitter.payWhitehat #14

Open
barakman opened this issue Jan 1, 2025 · 0 comments
Open

Splitter.payWhitehat #14

barakman opened this issue Jan 1, 2025 · 0 comments

Comments

@barakman
Copy link

barakman commented Jan 1, 2025

A few issues (though possibly all minor) around the following part in the aforementioned function:

for (uint256 i = 0; i < payout.length; i++) {
    uint256 feeAmount = (payout[i].amount * fee) / FEE_BASIS;
    if (feeAmount > 0) IERC20(payout[i].token).safeTransferFrom(msg.sender, feeRecipient, feeAmount);
    IERC20(payout[i].token).safeTransferFrom(msg.sender, wh, payout[i].amount);
}

The most critical issue is perhaps the transfer of payout[i].amount.
Shouldn't you be transferring only payout[i].amount - feeAmount?

Other issues are technically minor:

  1. You might want to consider replacing safeTransferFrom(msg.sender, ...) with safeTransfer(...), since the latter is cheaper and cleaner (and doesn't require a preliminary approval).
  2. You might want to consider checking payout[i].amount > 0 at the beginning of every iteration, in order to skip any redundant transfers (of zero). Presumably, passing zero amounts goes against the caller's incentive. But "causing damage with no practical gain" may nevertheless be an incentive. The damage in this case seems by itself minor - a redundant PayWhitehat event emitted in your contract. But perhaps some offchain entity relies on these events for some metrics or statistics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant