Sparkn

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

Transfer Failures due to Blacklisted ERC20 Addresses

maroutis

Summary

It is stated in the natspec that the protocol will only accept vetted tokens such as USDC, JPYCv1, JPYCv2, USDT, DAI, etc..

The _distribute function purpose is to distribute the respective compensation amounts to the winners. When payment token used is USDC (or any other token, stablecoin that implement a blacklist like functionality), if any involved actor is blacklisted, _distribute will revert, obstructing the other winners from receiving their rewards.

Vulnerability Details

There are two ways to send funds :

  • The organizer calls deployProxyAndDistribute or the owner calls deployProxyAndDistributeByOwner.

However, when the transfer fails, the owner/organizer will not know which address is causing the revert which will halt or block the whole process, especially if the winner's array is large.

POC: Transfer Failures

If a winner's address is blacklisted, token transfers to that address will fail.

// SPDX-License-Identifier: MIT
​
pragma solidity 0.8.18;
​
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {StdCheats} from "forge-std/StdCheats.sol";
import {HelperContract} from "./HelperContract.t.sol";
import {ProxyFactory} from "../../src/ProxyFactory.sol";
import {Proxy} from "../../src/Proxy.sol";
import {Distributor} from "../../src/Distributor.sol";
​
contract USDCMock is ERC20Mock {
error USDCMock__AccountBlackListed();
​
mapping(address => bool) public blacklisted;
​
function blacklist(address _account) external {
blacklisted[_account] = true;
}
​
modifier notBlacklisted(address _account) {
if (blacklisted[_account]) revert USDCMock__AccountBlackListed();
_;
}
​
function transfer(address recipient, uint256 amount)
public
virtual
override
notBlacklisted(recipient)
returns (bool)
{
return super.transfer(recipient, amount);
}
}
​
contract ProxyFactoryTestBlacklist is StdCheats, HelperContract {
ProxyFactory proxyFactoryTest;
address[] tokensToWhitelist = new address[](1);
Distributor distributorTest;
​
function setUp() public {
tokensToWhitelist[0] = address(new USDCMock());
vm.prank(factoryAdmin);
proxyFactoryTest = new ProxyFactory(tokensToWhitelist);
USDCMock(tokensToWhitelist[0]).mint(sponsor, 100000 ether);
distributorTest = new Distributor(address(proxyFactoryTest), stadiumAddress);
}
​
function createDataWithBlacklistedUser() public returns (bytes memory data) {
address[] memory tokens_ = new address[](1);
tokens_[0] = address(USDCMock(tokensToWhitelist[0]));
address[] memory winners = new address[](1);
winners[0] = user1;
// BlackList USER1
USDCMock(tokensToWhitelist[0]).blacklist(user1);
uint256[] memory percentages_ = new uint256[](1);
percentages_[0] = 9500;
data =
abi.encodeWithSelector(distributorTest.distribute.selector, tokensToWhitelist[0], winners, percentages_, "");
}
​
modifier setUpContestForJasonAndSentJpycv2Token(address _organizer) {
vm.startPrank(factoryAdmin);
bytes32 randomId = keccak256(abi.encode("Jason", "001"));
proxyFactoryTest.setContest(_organizer, randomId, block.timestamp + 8 days, address(distributorTest));
vm.stopPrank();
bytes32 salt = keccak256(abi.encode(_organizer, randomId, address(distributorTest)));
address proxyAddress = proxyFactoryTest.getProxyAddress(salt, address(distributorTest));
vm.startPrank(sponsor);
USDCMock(tokensToWhitelist[0]).transfer(proxyAddress, 10000 ether);
vm.stopPrank();
_;
}
​
function testDeployAndDistributesReverts_blackListedWinner()
public
setUpContestForJasonAndSentJpycv2Token(organizer)
{
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
bytes memory data = createDataWithBlacklistedUser();
​
vm.warp(9 days); // 9 days later
vm.startPrank(organizer);
// The error returned at a high level is ProxyFactory__DelegateCallFailed
// At a low level it will be USDCMock__AccountBlackListed
vm.expectRevert(ProxyFactory.ProxyFactory__DelegateCallFailed.selector);
proxyFactoryTest.deployProxyAndDistribute(randomId_, address(distributorTest), data);
vm.stopPrank();
​
// Results
// [605] USDCMock::transfer(0x000000000000000000000000000000000000000E, 9500000000000000000000 [9.5e21])
// │ │ │ │ └─ ← "USDCMock__AccountBlackListed()"
// │ │ │ └─ ← "USDCMock__AccountBlackListed()"
// │ │ └─ ← "USDCMock__AccountBlackListed()"
// │ └─ ← "ProxyFactory__DelegateCallFailed()"
// ├─ [0] VM::stopPrank()
// │ └─ ← ()
// └─ ← ()
}
}

Impact

The process will revert, but the caller of the _distribute function will not be able to know which address is causing the revert. If the winner's address is long, the distribute process will be completely halted.

Tools Used

Manual review and Foundry testing.

Recommendations

  • First recommandation: Right now the only message that gets logged after the transfer has failed is : "SafeERC20: ERC20 operation did not succeed". If the owners do not wish to change the code implementation strategy, they could change this message to log more information about the address or they could add the following revert after the line https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/Distributor.sol#L147 :

revert Distributor__TransferFailed(winners[i]);

This way, the caller can either remove the address, or ask the winner for a new address before creating the proxy and distributing the funds before calling the function again.

  • Second recommandation : Change the implementation strategy by using a pull pattern and allow each winner to withdraw their pay. However, by storing the rewards into a state variable, other proxy like related issues like storage variable clashing in the proxy contract need to be accounted for.

Support

FAQs

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

Give us feedback!