Summary
If the ERC20 token used for the contest's reward is having additional functionality that can make the token transfer revert (2 links added above), basically that will break the entire contest distribution process, as it will revert, even if only one transfer fails, which have multiples negavite impacts. But the worst will be in the case the protocol address (aka STADIUM_ADDRESS) is causing this revert, which is why this is submitted as a High (even if low probability), as this would make the funds unretrievable and lost.
Vulnerability Details
As a concrete example, USDC and USDT contract have a blacklist functionality, which allow those contracts to blacklist certain address according to their own criterias which we can't predict, and if any supporter or the STADIUM_ADDRESS is blacklisted, then the distribution process will revert. This affects all the distrdeployProxyAndDistribute* functions and distributeByOwner.
Impact
-
supporter being blacklisted:
The only way to retreive the funds in this case will be to wait for the contest to expire and then the owner will be able to call distributeByOwner to get the fund back.
-
STADIUM_ADDRESS being blacklisted:
This would be critical, as all the current deployed contests will be stuck with their funds not being able to be distributed to anyone, not even to the protocol. The only way out of this hole would be to have the token to unBlacklist the protocol address, but that is not something the protocol want to get into I assume.
PoC
I added this test testFailsIfBlacklisted in ProxyFactoryTest.t.sol and added blacklist functionality to MockERC20 as follow, which confirm the revert when calling distributeByOwner with protocol address being blacklisted.
function testFailsIfBlacklisted()
public
setUpContestForJasonAndSentJpycv2Token(organizer)
{
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 0 ether);
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 0 ether);
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
bytes32 salt_ = keccak256(abi.encode(organizer, randomId_, address(distributor)));
bytes memory data = createData();
address calculatedProxyAddress = proxyFactory.getProxyAddress(salt_, address(distributor));
vm.warp(16 days);
vm.startPrank(factoryAdmin);
address proxyAddress =
proxyFactory.deployProxyAndDistributeByOwner(organizer, randomId_, address(distributor), data);
vm.stopPrank();
assertEq(proxyAddress, calculatedProxyAddress);
vm.startPrank(sponsor);
MockERC20(jpycv2Address).transfer(proxyAddress, 10000 ether);
vm.stopPrank();
vm.warp(30 days);
MockERC20(jpycv2Address).blacklist(stadiumAddress);
bytes memory dataToSendToAdmin = createDataToSendToAdmin();
vm.startPrank(factoryAdmin);
proxyFactory.distributeByOwner(
calculatedProxyAddress, organizer, randomId_, address(distributor), dataToSendToAdmin
);
vm.stopPrank();
assertEq(MockERC20(jpycv2Address).balanceOf(user1), 9500 ether);
assertEq(MockERC20(jpycv2Address).balanceOf(stadiumAddress), 10500 ether);
}
contract MockERC20 is ERC20, Ownable {
error MockERC20__AmountMustBeMoreThanZero();
mapping(address => bool) private blacklisted;
* @dev Throws if argument account is blacklisted
* @param _account The address to check
*/
modifier notBlacklisted(address _account) {
require(
!blacklisted[_account],
"Blacklistable: account is blacklisted"
);
_;
}
constructor(string memory name, string memory symbol) ERC20(name, symbol) {
_mint(msg.sender, 100000 * 10 ** decimals());
}
function mint(address _to, uint256 _amount) external onlyOwner returns (bool) {
if (_amount == 0) {
revert MockERC20__AmountMustBeMoreThanZero();
}
_mint(_to, _amount);
return true;
}
function blacklist(address _badboy) external {
blacklisted[_badboy] = true;
}
function transfer(address to, uint256 amount)
public
override
notBlacklisted(msg.sender)
notBlacklisted(to)
returns (bool)
{
super.transfer(to, amount);
return true;
}
}
Tools Used
VS code and test suite.
Recommendations
A supporter being blacklisted should not prevent the whole distribution. So for example you could use transfer instead of safeTransfer and use a try/catch to avoid the revert (as shown below), and probably send this failed percentage to the STADIUM_ADDRESS.
The STADIUM_ADDRESS being blacklisted is more critical and tricky to address. I would probably add a function in ProxyFactory with an onlyOwner that would allow to update the protocol address of a deployed contest (which would make it not immutable, granted)
@@ -144,7 +144,12 @@ contract Distributor {
uint256 winnersLength = winners.length; // cache length
for (uint256 i; i < winnersLength;) {
uint256 amount = totalAmount * percentages[i] / BASIS_POINTS;
- erc20.safeTransfer(winners[i], amount);
+
+ try erc20.transfer(winners[i], amount) {
+ } catch {
+ }
+
unchecked {
++i;
Informational
Please remove this magic number.
@@ -126,17 +126,17 @@ contract Distributor {
uint256 percentagesLength = percentages.length;
uint256 totalPercentage;
for (uint256 i; i < percentagesLength;) {
totalPercentage += percentages[i];
unchecked {
++i;
}
}
// check if totalPercentage is correct
- if (totalPercentage != (10000 - COMMISSION_FEE)) {
+ if (totalPercentage != (BASIS_POINTS - COMMISSION_FEE)) {
revert Distributor__MismatchedPercentages();
}
IERC20 erc20 = IERC20(token);
uint256 totalAmount = erc20.balanceOf(address(this));
// if there is no token to distribute, then revert
if (totalAmount == 0) revert Distributor__NoTokenToDistribute();
