Submission Details
Severity:
`0` value transfers may do event poisoning and even revert
Summary
There is no sanity check that the percentages nor the amounts are different than 0, therefore, if some winner is included with a value of 0, the whole operation would revert for some tokens that revert on 0 value transfers
Vulnerability Details
Distributions use the following formula uint256 amount = (totalAmount * percentages[i]) / BASIS_POINTS; if one of the percentages is 0, the whole value of the operation will be 0, and therefore, a transfer of 0 value will occur
function _distribute(address token, address[] memory winners, uint256[] memory percentages, bytes memory data)
...
for (uint256 i; i < winnersLength; ) {
uint256 amount = (totalAmount * percentages[i]) / BASIS_POINTS; // @audit no validation != 0
erc20.safeTransfer(winners[i], amount); //@audit can revert
unchecked {
++i;
}
}
...
### PoC:
This test file includes a tests for erc20 tokens with block list, and with tokens that revert on transfers of 0 value.
// SPDX-License-Identifier: MIT
import {MockERC20} from "../test/mock/MockERC20.sol";
import {ECDSA} from "openzeppelin/utils/cryptography/ECDSA.sol";
import {Test, console} from "forge-std/Test.sol";
import {StdCheats} from "forge-std/StdCheats.sol";
import {ProxyFactory} from "../src/ProxyFactory.sol";
import {Proxy} from "../src/Proxy.sol";
import {Distributor} from "../src/Distributor.sol";
import {HelperContract} from "../test/integration/HelperContract.t.sol";
import {Script} from "forge-std/Script.sol";
import {HelperConfig} from "script/HelperConfig.s.sol";
contract DeployContracts is Script {
// tokens' array to whitelist
address public stadiumAddress = makeAddr("stadium");
address public factoryAdmin = makeAddr("factoryAdmin");
function run() external returns (ProxyFactory, Distributor, HelperConfig) {
// set up config
HelperConfig config = new HelperConfig();
// get the addresses of the tokens to whitelist
(
address jpycv1Address,
address jpycv2Address,
address usdcAddress,
,
uint256 deployerKey
) = config.activeNetworkConfig();
// whitelist 3 kinds of tokens
address[] memory tokensToWhitelist = new address[](5);
tokensToWhitelist[0] = jpycv1Address;
tokensToWhitelist[1] = jpycv2Address;
tokensToWhitelist[2] = usdcAddress;
// address(uint160(uint256(keccak256(abi.encodePacked("No Zeros ERC20")))))
tokensToWhitelist[3] = 0xF9ee2486f2Aaf8245FFf41Ff18f40776dCc11a6d;
// address(uint160(uint256(keccak256(abi.encodePacked("Banlist ERC20")))))
tokensToWhitelist[4] = 0xAcB40B1fBFFD23F4A99F32F12662B92B2E0c37C2;
vm.startBroadcast(deployerKey); // prank
ProxyFactory proxyFactory = new ProxyFactory(tokensToWhitelist);
proxyFactory.transferOwnership(factoryAdmin);
Distributor distributor = new Distributor(
address(proxyFactory),
stadiumAddress
);
vm.stopBroadcast();
return (proxyFactory, distributor, config);
}
}
contract MyHelperContract is HelperContract {
constructor() {
DeployContracts deployContracts = new DeployContracts();
(proxyFactory, distributor, config) = deployContracts.run();
(
jpycv1Address,
jpycv2Address,
usdcAddress,
usdtAddress,
deployerKey
) = config.activeNetworkConfig();
}
}
contract NOZEROSERC20 is MockERC20 {
error NOZEROSERC20__TransferAmountMustBeMoreThanZero();
constructor() MockERC20("NOZEROSERC20", "NZS") {}
// Modified from https://github.com/d-xo/weird-erc20/blob/main/src/RevertZero.sol
function transferFrom(
address from,
address to,
uint256 value
) public override returns (bool) {
if (value == 0) {
revert NOZEROSERC20__TransferAmountMustBeMoreThanZero();
}
return super.transferFrom(from, to, value);
}
function transfer(
address to,
uint256 value
) public override returns (bool) {
if (value == 0) {
revert NOZEROSERC20__TransferAmountMustBeMoreThanZero();
}
return super.transfer(to, value);
}
}
contract BANERC20 is MockERC20 {
error BANERC20__AccountBlacklisted();
mapping(address => bool) internal blacklisted;
event Banned(address indexed _account);
event UnBanned(address indexed _account);
constructor() MockERC20("BANERC20", "BAN") {}
modifier notBanned(address _account) {
if (blacklisted[_account]) {
revert BANERC20__AccountBlacklisted();
}
_;
}
function isBanned(address _account) external view returns (bool) {
return blacklisted[_account];
}
function Ban(address _account) external {
blacklisted[_account] = true;
emit Banned(_account);
}
function unBan(address _account) external {
blacklisted[_account] = false;
emit UnBanned(_account);
}
function transferFrom(
address from,
address to,
uint256 value
)
public
override
notBanned(msg.sender)
notBanned(from)
notBanned(to)
returns (bool)
{
return super.transferFrom(from, to, value);
}
function transfer(
address to,
uint256 value
) public override notBanned(msg.sender) notBanned(to) returns (bool) {
return super.transfer(to, value);
}
}
contract ProxyFactoryTest is StdCheats, MyHelperContract {
// address(uint160(uint256(keccak256(abi.encodePacked("No Zeros ERC20")))))
address noZerosERC20 = 0xF9ee2486f2Aaf8245FFf41Ff18f40776dCc11a6d;
address banlistERC20 = 0xAcB40B1fBFFD23F4A99F32F12662B92B2E0c37C2;
function setUp() public {
// set up balances of each token belongs to each user
if (block.chainid == 31337) {
// deal ether
vm.deal(factoryAdmin, STARTING_USER_BALANCE);
vm.deal(sponsor, SMALL_STARTING_USER_BALANCE);
vm.deal(organizer, SMALL_STARTING_USER_BALANCE);
vm.deal(user1, SMALL_STARTING_USER_BALANCE);
vm.deal(user2, SMALL_STARTING_USER_BALANCE);
vm.deal(user3, SMALL_STARTING_USER_BALANCE);
vm.deal(TEST_SIGNER, SMALL_STARTING_USER_BALANCE);
// mint erc20 token
vm.etch(noZerosERC20, address(new NOZEROSERC20()).code);
vm.etch(banlistERC20, address(new BANERC20()).code);
deal(noZerosERC20, sponsor, 100_000 ether);
deal(noZerosERC20, sponsor, 300_000 ether);
deal(noZerosERC20, organizer, 100_000 ether);
deal(noZerosERC20, organizer, 300_000 ether);
deal(noZerosERC20, TEST_SIGNER, 100_000 ether);
deal(noZerosERC20, TEST_SIGNER, 300_000 ether);
deal(banlistERC20, sponsor, 100_000 ether);
deal(banlistERC20, sponsor, 300_000 ether);
deal(banlistERC20, organizer, 100_000 ether);
deal(banlistERC20, organizer, 300_000 ether);
deal(banlistERC20, TEST_SIGNER, 100_000 ether);
deal(banlistERC20, TEST_SIGNER, 300_000 ether);
}
// labels
vm.label(organizer, "organizer");
vm.label(sponsor, "sponsor");
vm.label(supporter, "supporter");
vm.label(user1, "user1");
vm.label(user2, "user2");
vm.label(user3, "user3");
}
modifier setUpContestForJasonAndSentToken(
address _organizer,
address token
) {
vm.startPrank(factoryAdmin);
bytes32 randomId = keccak256(abi.encode("Jason", "001"));
proxyFactory.setContest(
_organizer,
randomId,
block.timestamp + 8 days,
address(distributor)
);
vm.stopPrank();
bytes32 salt = keccak256(
abi.encode(_organizer, randomId, address(distributor))
);
address proxyAddress = proxyFactory.getProxyAddress(
salt,
address(distributor)
);
vm.startPrank(sponsor);
MockERC20(token).transfer(proxyAddress, 10000 ether);
vm.stopPrank();
assertEq(MockERC20(token).balanceOf(proxyAddress), 10000 ether);
_;
}
function createData(address token) public view returns (bytes memory data) {
address[] memory tokens_ = new address[](1);
tokens_[0] = token;
address[] memory winners = new address[](2);
winners[0] = user1;
winners[1] = user2;
uint256[] memory percentages_ = new uint256[](2);
percentages_[0] = 9500;
percentages_[1] = 0;
data = abi.encodeWithSelector(
Distributor.distribute.selector,
token,
winners,
percentages_,
""
);
}
function testDOSWhenZeroPercentOnUser()
public
setUpContestForJasonAndSentToken(organizer, noZerosERC20)
{
// before
assertEq(MockERC20(noZerosERC20).balanceOf(user1), 0 ether);
assertEq(MockERC20(noZerosERC20).balanceOf(user2), 0 ether);
assertEq(MockERC20(noZerosERC20).balanceOf(stadiumAddress), 0 ether);
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
bytes memory data = createData(noZerosERC20);
vm.warp(9 days); // 9 days later
vm.startPrank(organizer);
vm.expectRevert(ProxyFactory.ProxyFactory__DelegateCallFailed.selector);
proxyFactory.deployProxyAndDistribute(
randomId_,
address(distributor),
data
);
vm.stopPrank();
}
function testSucessWithBanlistToken()
public
setUpContestForJasonAndSentToken(organizer, banlistERC20)
{
// before
assertEq(MockERC20(banlistERC20).balanceOf(user1), 0 ether);
assertEq(MockERC20(banlistERC20).balanceOf(user2), 0 ether);
assertEq(MockERC20(banlistERC20).balanceOf(stadiumAddress), 0 ether);
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
bytes memory data = createData(banlistERC20);
vm.warp(9 days); // 9 days later
vm.startPrank(organizer);
proxyFactory.deployProxyAndDistribute(
randomId_,
address(distributor),
data
);
vm.stopPrank();
// after
assertEq(MockERC20(banlistERC20).balanceOf(user1), 9500 ether);
assertEq(MockERC20(banlistERC20).balanceOf(user2), 0 ether);
assertEq(MockERC20(banlistERC20).balanceOf(stadiumAddress), 500 ether);
}
function testDOSWhenBannedUser()
public
setUpContestForJasonAndSentToken(organizer, banlistERC20)
{
// before
assertEq(MockERC20(banlistERC20).balanceOf(user1), 0 ether);
assertEq(MockERC20(banlistERC20).balanceOf(user2), 0 ether);
assertEq(MockERC20(banlistERC20).balanceOf(stadiumAddress), 0 ether);
// ban user1
BANERC20(banlistERC20).Ban(user1);
bytes32 randomId_ = keccak256(abi.encode("Jason", "001"));
bytes memory data = createData(banlistERC20);
vm.warp(9 days); // 9 days later
vm.startPrank(organizer);
vm.expectRevert(ProxyFactory.ProxyFactory__DelegateCallFailed.selector);
proxyFactory.deployProxyAndDistribute(
randomId_,
address(distributor),
data
);
vm.stopPrank();
}
}
The tests are using expectRevert and passing as expected:
Running 3 tests for tcustom/PoCs.t.sol:ProxyFactoryTest
PASS testDOSWhenBannedUser() (gas: 210440)
PASS testDOSWhenZeroPercentOnUser() (gas: 205907)
PASS testSucessWithBanlistToken() (gas: 238620)
Impact
In the rare case some stable coin that follow this requirement is used at distribute, it will not work on this 0 value cases. It will also create some event poisoning as this transfers would have 0 value but would emit the event no matter what.
Tools Used
Manual Review
Recommendations
Add a flow that checks for
amount(maybe forpercentage[i]) not to be 0 before doing the transfer
Example:
uint256 amount = (totalAmount * percentages[i]) / BASIS_POINTS;
- erc20.safeTransfer(winners[i], amount);
+ if (amount != 0) {
+ erc20.safeTransfer(winners[i], amount);
+ }
Prize pool breakdown
Total prize
15,000 USDC
nSLOC
19477 USDC / LOC
14,000 USDC
1,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.