Sparkn

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

Funds can be lost in case winners == address(0).

mrjorystewartbaxter

Summary

If the winners’ addresses are erroneously set with the zero address, the distribution will result in a loss of funds.

Vulnerability Details

The contract can be successfully deployed with a zero address for the winners (aka supporters), which should not be allowed.

In the contracts, there are specific checks for nonzero address in

the organizer and implementation address: https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L109

the token address:
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L120

the factory and stadium address:
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L77

the proxy address:
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L212

and the whitelisted tokens addresses:
https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/ProxyFactory.sol#L84

But even though there is a specific check that reverts when the winners array length is equal to 0 (https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L125), the winner’s addresses itself are not verified against the zero address.

Consider the following PoC using the HelperContract.t.sol. As mentioned it is possible to make a deployment using the address(0) for the winners as nothing prevents to do it and there is no check implemented; therefore we can make a minor modification to change the variable user1 to address(0), where all the actors are declared to test our scenario:

// users
address public stadiumAddress = makeAddr("stadium");
address public factoryAdmin = makeAddr("factoryAdmin");
address public tokenMinter = makeAddr("tokenMinter");
address public organizer = address(11);
address public sponsor = address(12);
address public supporter = address(13);
​
// Change user1 to have address(0)
address public user1 = address(0);
address public user2 = address(15);
address public user3 = address(16);

After executing the test where all conditions are met, so the contest should succeed testIfAllConditionsMetThenUsdcSendingCallShouldSuceed, we get a revert when distributing the prizes because one of the winners is having an address(0), which we know in advance is user1, as this user is used throughout the function test:

[187707] ProxyTest::testIfAllConditionsMetThenUsdcSendingCallShouldSuceed()
​
///// output not displayed
​
[11147] 0x9824278B4b41B8f00c96f05BA590Af1a5cE0E326::distribute(MockERC20: [0x90193C961A926261B756D1E5bb255e67ff9498A1], [0x0000000000000000000000000000000000000000], [9500], 0x)
│ │ ├─ [8345] Distributor::distribute(MockERC20: [0x90193C961A926261B756D1E5bb255e67ff9498A1], [0x0000000000000000000000000000000000000000], [9500], 0x) [delegatecall]
│ │ │ ├─ [2575] ProxyFactory::whitelistedTokens(MockERC20: [0x90193C961A926261B756D1E5bb255e67ff9498A1]) [staticcall]
│ │ │ │ └─ ← true
│ │ │ ├─ [563] MockERC20::balanceOf(0x9824278B4b41B8f00c96f05BA590Af1a5cE0E326) [staticcall]
│ │ │ │ └─ ← 10000000000000000000000 [1e22]
│ │ │ ├─ [597] MockERC20::transfer(user1: [0x0000000000000000000000000000000000000000], 9500000000000000000000 [9.5e21])
│ │ │ │ └─ ← "ERC20: transfer to the zero address"
│ │ │ └─ ← "ERC20: transfer to the zero address"
│ │ └─ ← "ERC20: transfer to the zero address"
│ └─ ← "ProxyFactory__DelegateCallFailed()"
└─ ← "ProxyFactory__DelegateCallFailed()"

Impact

Sponsor funds can get lost in contract if winners’ addresses are erroneously set as a zero address.

Tools Used

Static review

Recommendations

There should be zero address checkings in place for the winners (supporters) to prevent loss of funds.

Support

FAQs

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