Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: high

Design flaw in the _distribute() can unexpectedly drain the rescue requestor's stuck token

serialcoder

Summary

The Distributor::_distribute() does not account for the stuck amount mistakenly transferred to the Proxy contract. If the stuck token's address is identical to the prize token's address, the stuck amount will be distributed to contest winners.

Subsequently, a rescue requestor will lose their token permanently.

Vulnerability Details

The Sparkn protocol was designed as an escrow for distributing prizes (tokens) to contest winners. One of the protocol features is recovering stuck tokens (being sent by mistake) from a contest's Proxy contract after the contest expires.

In other words, all prizes must be distributed to all winners before an owner (protocol admin) can execute the stuck tokens' recovery process. However, if the stuck token's address is identical to the prize token's address, the stuck amount will not be accounted for by the _distribute().

The _distribute() will count the total balance locked in the Proxy as prizes for winners. Therefore, the stuck amount will also be distributed to the contest winners. The remaining amount will then become the protocol's commission fee and be transferred to the protocol's fee collector.

Consequently, a rescue requestor could not retrieve their token as expected.

function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
internal
{
...
​
IERC20 erc20 = IERC20(token);
@> uint256 totalAmount = erc20.balanceOf(address(this));
​
...
​
uint256 winnersLength = winners.length; // cache length
for (uint256 i; i < winnersLength;) {
@> uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
@> erc20.safeTransfer(winners[i], amount);
unchecked {
++i;
}
}
​
...
​
@> _commissionTransfer(erc20);
emit Distributed(token, winners, percentages, data);
}
​
...
​
function _commissionTransfer(IERC20 token) internal {
@> token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
}

  • The total token balance is viewed as winners' prizes: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L139

  • All balance (minus the commission fee) is distributed to winners: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L146-L147

  • Transferring the commission fee: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L154

  • The commission fee is transferred to the protocol's fee collector: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L164

Impact

Suppose the stuck token's address is identical to the prize token's address. In that case, the stuck amount will be distributed to contest winners unexpectedly since the stuck amount will be counted as prizes for the winners.

Subsequently, a rescue requestor will lose their token permanently.

Tools Used

Manual Review

Recommendations

To fix the vulnerability, I recommend improving the following functions: distribute(), _distribute(), and _commissionTransfer(), as shown below.

  • The uint256 maxAmountToTransfer param is introduced to the distribute(), which must also be passed to the _distribute().

  • The _distribute() must count the maxAmountToTransfer param as the maximum prize amount to distribute to winners instead.

  • The _commissionTransfer() must transfer only a commission fee calculated from the maxAmountToTransfer param instead of transferring all remaining amount.

This way, an organizer or owner (protocol admin) can define the maxAmountToTransfer param when executing the distribute(). The stuck amount will not be mistakenly transferred from the Proxy and can be retrieved to a rescue requestor as expected.

// distribute()
- function distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+ function distribute(address token, uint256 maxAmountToTransfer, address[] memory winners, uint256[] memory percentages, bytes memory data) //@audit -- the maxAmountToTransfer param is introduced
external
{
if (msg.sender != FACTORY_ADDRESS) {
revert Distributor__OnlyFactoryAddressIsAllowed();
}
- _distribute(token, winners, percentages, data);
+ _distribute(token, maxAmountToTransfer, winners, percentages, data); //@audit -- the maxAmountToTransfer param must be passed to the _distribute()
}
​
...
// _distribute()
- function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
+ function _distribute(address token, uint256 maxAmountToTransfer, address[] memory winners, uint256[] memory percentages, bytes memory data)
internal
{
...
​
IERC20 erc20 = IERC20(token);
uint256 totalAmount = erc20.balanceOf(address(this));
​
//@audit -- do some input checks on the maxAmountToTransfer param
+ if (maxAmountToTransfer == 0) revert Distributor__MaxAmountToTransferIsZero();
+ if (maxAmountToTransfer > totalAmount) revert Distributor__InsufficientAmountToDistribute();
​
...
​
uint256 winnersLength = winners.length; // cache length
for (uint256 i; i < winnersLength;) {
- uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
+ uint256 amount = maxAmountToTransfer * percentages[i] / BASIS_POINTS; //@audit -- count the maxAmountToTransfer param as the maximum prize amount to distribute to winners
erc20.safeTransfer(winners[i], amount);
unchecked {
++i;
}
}
​
...
​
- _commissionTransfer(erc20);
+ _commissionTransfer(erc20, maxAmountToTransfer); //@audit -- the maxAmountToTransfer param must be passed to the _commissionTransfer()
emit Distributed(token, winners, percentages, data);
}
​
...
​
// _commissionTransfer()
- function _commissionTransfer(IERC20 token) internal {
+ function _commissionTransfer(IERC20 token, uint256 maxAmountToTransfer) internal {
​
//@audit -- the _commissionTransfer() must transfer only a commission fee calculated from the maxAmountToTransfer param instead of transferring all remaining amount
+ uint256 amount = maxAmountToTransfer * COMMISSION_FEE / BASIS_POINTS;
​
- token.safeTransfer(STADIUM_ADDRESS, token.balanceOf(address(this)));
+ token.safeTransfer(STADIUM_ADDRESS, amount);
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.