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

[codehawks-bot]

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

Severity

High Risk

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.

[asobiesk]

[APPEAL] This finding seems invalid as no fund loss occurs in this scenario.

As the POC proves, when the organizer calls the ProxyFactory#deployProxyAndDistribute() method, the transaction reverts. But no tokens are getting stuck or incorrectly transferred.

Here is how the method looks like:

 /**
     * @notice deploy proxy contract and distribute winner's prize
     * @dev the caller can only control his own contest
     * @param contestId The contest id
     * @param implementation The implementation address
     * @param data The prize distribution data
     * @return The proxy address
     */
    function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
        public
        returns (address)
    {
        bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
        if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
        // can set close time to current time and end it immediately if organizer wish
        //@audit-issue The comment above is not true. The organizer has to wait!
        if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
        address proxy = _deployProxy(msg.sender, contestId, implementation);
        _distribute(proxy, data);
        return proxy;
    }

If the data includes address(0), then the _distribute() call will revert - as proven in the POC. That means this whole transaction will revert, including the proxy deployment.

After this happens, the organizer may simply call the method again, this time without the problematic address(0) - and the funds will get distributed correctly. The only loss that occurs in this scenario is the gas cost for the organizer which initially called the method with incorrect parameter.

[B353N]

[APPEAL] This finding seems invalid as no fund loss occurs in this scenario.

As the POC proves, when the organizer calls the ProxyFactory#deployProxyAndDistribute() method, the transaction reverts. But no tokens are getting stuck or incorrectly transferred.

Here is how the method looks like:

 /**
     * @notice deploy proxy contract and distribute winner's prize
     * @dev the caller can only control his own contest
     * @param contestId The contest id
     * @param implementation The implementation address
     * @param data The prize distribution data
     * @return The proxy address
     */
    function deployProxyAndDistribute(bytes32 contestId, address implementation, bytes calldata data)
        public
        returns (address)
    {
        bytes32 salt = _calculateSalt(msg.sender, contestId, implementation);
        if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
        // can set close time to current time and end it immediately if organizer wish
        //@audit-issue The comment above is not true. The organizer has to wait!
        if (saltToCloseTime[salt] > block.timestamp) revert ProxyFactory__ContestIsNotClosed();
        address proxy = _deployProxy(msg.sender, contestId, implementation);
        _distribute(proxy, data);
        return proxy;
    }

If the data includes address(0), then the _distribute() call will revert - as proven in the POC. That means this whole transaction will revert, including the proxy deployment.

After this happens, the organizer may simply call the method again, this time without the problematic address(0) - and the funds will get distributed correctly. The only loss that occurs in this scenario is the gas cost for the organizer which initially called the method with incorrect parameter.

Where you see this check, if data include address(0) to revert? There no have such a check , and transaction will not revert. As this will lose winner tokens!

[asobiesk]

POC itself demonstrates that revert. The revert I am talking about is happening inside the ERC20's transfer. Note also that the OZ safeTransfer is used to execute the transfer.

[dontonka]

Yes I also concurs with asobiesk, I saw this very quickly in the audit but discarded right away as safeTransfer will always revert on address(0) transfer.

The revert will happen here: Distributor.sol

erc20.safeTransfer(winners[i], amount);

ERC20.sol

    function _transfer(address from, address to, uint256 amount) internal virtual {
        require(from != address(0), "ERC20: transfer from the zero address");
        require(to != address(0), "ERC20: transfer to the zero address");

        _beforeTokenTransfer(from, to, amount);

        uint256 fromBalance = _balances[from];
        require(fromBalance >= amount, "ERC20: transfer amount exceeds balance");
        unchecked {
            _balances[from] = fromBalance - amount;
            // Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
            // decrementing then incrementing.
            _balances[to] += amount;
        }

        emit Transfer(from, to, amount);

        _afterTokenTransfer(from, to, amount);
    }

[asobiesk]

Okay, I've been thinking about what you have said on Discord @B353N and I think we are all patrially right and partially wrong.

The Distributor contract indeed uses OpenZeppelin's safeTransfer, but that only means it will revert if the token's transfer returned false. @dontonka has mentioned the ERC20.sol code, but the Distributor code uses IERC20.sol to wrap the token's address. As such, it does not mean that the actual token's implementation will be alligned with the OZ's standard.

There still may be a token that will not revert on transfer to zero address and will return true. DAI is an example of such a token. If DAI is used in the price pool, the transfer to address zero will indeed succeed.

Having said that, I still argue that this particular issue is incorrect - it clearly mentions revert as a proof of token loss, while the token loss occur in a completely opposite scenario - when there is no revert. However, there is one potentially valid finding under the label - #721. This seems to be the only one that mentions the actual issue.

Those findings may still be deemed invalid based on the fact that the organizer is a trusted role, so their mistakes are out-of-scope. This probably will be a tough call for judges as docs only mention owner as a trusted role, but organizer was also stated to be trusted on Discord by the contest's sponsors (and the fact that they are trusted may be implicitly deducted from the known issue "We may build a reputation system in the futue to handle the issue of the system being maliciously used, e.g., sybil attack."). This dillema is likely going to appear in multiple appeals.

If the judges decide that the organizer is not trusted, then I would say #721 should be accepted as a valid solo medium.

[quentin-abei]

Okay, I've been thinking about what you have said on Discord @B353N and I think we are all patrially right and partially wrong.

The Distributor contract indeed uses OpenZeppelin's safeTransfer, but that only means it will revert if the token's transfer returned false. @dontonka has mentioned the ERC20.sol code, but the Distributor code uses IERC20.sol to wrap the token's address. As such, it does not mean that the actual token's implementation will be alligned with the OZ's standard.

There still may be a token that will not revert on transfer to zero address and will return true. DAI is an example of such a token. If DAI is used in the price pool, the transfer to address zero will indeed succeed.

Having said that, I still argue that this particular issue is incorrect - it clearly mentions revert as a proof of token loss, while the token loss occur in a completely opposite scenario - when there is no revert. However, there is one potentially valid finding under the label - #721. This seems to be the only one that mentions the actual issue.

Those findings may still be deemed invalid based on the fact that the organizer is a trusted role, so their mistakes are out-of-scope. This probably will be a tough call for judges as docs only mention owner as a trusted role, but organizer was also stated to be trusted on Discord by the contest's sponsors (and the fact that they are trusted may be implicitly deducted from the known issue "We may build a reputation system in the futue to handle the issue of the system being maliciously used, e.g., sybil attack."). This dillema is likely going to appear in multiple appeals.

If the judges decide that the organizer is not trusted, then I would say #721 should be accepted as a valid solo medium.

You are wrong, the issue state ithe impact is that sponsors funds will be lost if winner address is zero address, please read the impact again . It's that and not because token transfer will revert . There is a slight wrong thing in PoC because it used a standard ERC20 and not one that will allow transfer to zero address such as DAI . That being said this issue is valid as the impact of setting address to zero is that sponsors can lose fund ( although it's not mentionned that specific tokens as DAI will cause this lost of funds ) but it's still valid. Judge may be look for another duplicate that states this for selection. Overral Issue is valid.

[0xcats]

@dontonka DAI is indeed listed as a reference in the whitelisted tokens. Furthermore, we do not have a confirmed list of whitelisted tokens that will be used, but only an “example”. It is possible there are more tokens that potentially would’ve been used, which there transfers would not revert, if not for the audit shedding light. Also, not all of the issue’s duplicates mention the loss of funds upon revert instead of on send.

Due to what I highlighted above and what I’ve seen across different judging phases and contests, I haven’t encountered a case where all issues with tokens that revert are invalidated whilst the ones that go through are validated, the fact of the matter is that a scenario/s that leads to loss of funds due to this reason exists, which could be very easily mitigated with a check for address(0). The rest I leave to the judges.

[ArnieSec]

First this issue is caused by input error, yes it is great to have a check that can prevent this input error. This does not mean that the issue above is a valid high. Funds can also be lost if organizer inputs the incorrect addresses. Should we also have another check to ensure all addresses are correct, how would that even work? I feel like missing 0 address checks issues should at most be medium, if not always low. Especially in this scenario where the organizer is assumed to be trusted. It is weird to trust an organizer with the disbursal of funds but then later say, actually we don't trust that you might mess up the addresses and input a 0 address.

From the codehawks docs, we can reference the matrix...

Likelihood is low. impact, ill be fair and say high.

According to the codehawks docs, this at most should be a medium. I am leaning towards low.

[dontonka]

@dontonka DAI is indeed listed as a reference in the whitelisted tokens. Furthermore, we do not have a confirmed list of whitelisted tokens that will be used, but only an “example”. It is possible there are more tokens that potentially would’ve been used, which there transfers would not revert, if not for the audit shedding light. Also, not all of the issue’s duplicates mention the loss of funds upon revert instead of on send.

Due to what I highlighted above and what I’ve seen across different judging phases and contests, I haven’t encountered a case where all issues with tokens that revert are invalidated whilst the ones that go through are validated, the fact of the matter is that a scenario/s that leads to loss of funds due to this reason exists, which could be very easily mitigated with a check for address(0). The rest I leave to the judges.

Agreed. My mistake and lack of knowledge here, missed this was only an interface and the actual ERC20 implementation might have such flaw, and didn't knew DAI was a clear example of that, good learning for me, thank you XD.

[justefg]

this is a valid finding however an organiser can mistakenly put a random non-zero address. address(0) just means funds are gone forever. if you put a wrong non-zero address a slim chance someone controls the corresponding private key and is willing to return the funds. my point is checking for address(0) won't really do much. still nice to have though. overall it's a no brainer check on the frontend side anyways or whatever means an organiser uses to close the contest.

[0xhahax0]

Organizers are trusted. Owners are trusted. That alone invalidates this issue.

See: Organizers are trusted

[0xvangrim]

I agree with @justefg - and this issue should be downgraded to low, info, or invalidated altogether. While doing a require winner != address(0) is standard, the root cause is still a user input error. This should also be in line with the discussions held here: https://github.com/Cyfrin/2023-08-sparkn/issues/897 of user input error not being able to go as high/medium findings.

If we recognize the user input errors, then the same high/medium findings could pertain to a random non-zero address address(1), address(2), etc. Or just any mistakenly inputted address to someone who did not participate and win the contest.

[fvelazquez-X]

As I described in the findings one must refer directly to the code itself to understand and validate the issue and precisely not to make assumptions or subjective rethoric of "trusted" or "not trusted" because those are only assumptions per se, which are not valid at all.

The code itself has zero address checkings for:

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

which is exactly why they need to verify winners. Moreover, out there exists vast documentation regarding strict policies in relation of never allowing zero addresses (a.k.a https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222) among others and in several other audits, this checking is crucial (depending of course on the context given) but without a doubt it should be considered as an important issue. Examples:

https://solodit.xyz/issues/h-01-someone-can-create-non-liquidatable-auction-if-the-collateral-asset-fails-on-transferring-to-address0-code4rena-yield-yield-witch-v2-contest-git

https://solodit.xyz/issues/h-1-attackers-can-use-reclaimcontract-to-transfer-assets-in-protocol-to-address0-sherlock-bullvbear-bull-v-bear-git

Honestly arguing just by saying trusted not trusted, or even without referring to the code or by implying human error diminishes the fact that we are talking about smart contracts, everything should be checked on chain, on the code, and this is one of the basic premises here. If not, then better to write a document if you want to make assumptions instead of a smart contract.

Regards,

[0xhahax0]

As I described in the findings one must refer directly to the code itself to understand and validate the issue and precisely not to make assumptions or subjective rethoric of "trusted" or "not trusted" because those are only assumptions per se, which are not valid at all.

The code itself has zero address checkings for:

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

which is exactly why they need to verify winners. Moreover, out there exists vast documentation regarding strict policies in relation of never allowing zero addresses (a.k.a https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222) among others and in several other audits, this checking is crucial (depending of course on the context given) but without a doubt it should be considered as an important issue. Examples:

https://solodit.xyz/issues/h-01-someone-can-create-non-liquidatable-auction-if-the-collateral-asset-fails-on-transferring-to-address0-code4rena-yield-yield-witch-v2-contest-git

https://solodit.xyz/issues/h-1-attackers-can-use-reclaimcontract-to-transfer-assets-in-protocol-to-address0-sherlock-bullvbear-bull-v-bear-git

Honestly arguing just by saying trusted not trusted, or even without referring to the code or by implying human error diminishes the fact that we are talking about smart contracts, everything should be checked on chain, on the code, and this is one of the basic premises here. If not, then better to write a document if you want to make assumptions instead of a smart contract.

Regards,

Following this line of thought, known-issues should be submitted as bugs as well because.. code == code. This does not work in audit contests.

I agree with the general idea of auditing the code, not human assumptions.

However, people get punished for having too many invalid issues as per CodeHawks docs. This means that whenever a sponsors communicates or sets a restraint, you are expected not to report the restraint as a valid issue because it could invalidate your correct ones. There's a reason why people don't report known issues.

I do agree that in platforms like Immumefi, code should be code and that's it. If you find a bug, it's a bug.

[fvelazquez-X]

As I described in the findings one must refer directly to the code itself to understand and validate the issue and precisely not to make assumptions or subjective rethoric of "trusted" or "not trusted" because those are only assumptions per se, which are not valid at all. The code itself has zero address checkings for: 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 which is exactly why they need to verify winners. Moreover, out there exists vast documentation regarding strict policies in relation of never allowing zero addresses (a.k.a https://forum.openzeppelin.com/t/removing-address-0x0-checks-from-openzeppelin-contracts/2222) among others and in several other audits, this checking is crucial (depending of course on the context given) but without a doubt it should be considered as an important issue. Examples: https://solodit.xyz/issues/h-01-someone-can-create-non-liquidatable-auction-if-the-collateral-asset-fails-on-transferring-to-address0-code4rena-yield-yield-witch-v2-contest-git https://solodit.xyz/issues/h-1-attackers-can-use-reclaimcontract-to-transfer-assets-in-protocol-to-address0-sherlock-bullvbear-bull-v-bear-git Honestly arguing just by saying trusted not trusted, or even without referring to the code or by implying human error diminishes the fact that we are talking about smart contracts, everything should be checked on chain, on the code, and this is one of the basic premises here. If not, then better to write a document if you want to make assumptions instead of a smart contract. Regards,

Following this line of thought, known-issues should be submitted as bugs as well because.. code == code. This does not work in audit contests.

I agree with the general idea of auditing the code, not human assumptions.

However, people get punished for having too many invalid issues as per CodeHawks docs. This means that whenever a sponsors communicates or sets a restraint, you are expected not to report the restraint as a valid issue because it could invalidate your correct ones. There's a reason why people don't report known issues.

I do agree that in platforms like Immumefi, code should be code and that's it. If you find a bug, it's a bug.

Following this line of thought, known-issues should be submitted as bugs as well because.. code == code. This does not work in audit contests. // I do not know if all known issues, this is just considering the context of this particular code and taking into account the inners of the application itself. Thats why when quoting the examples in solodit I highlighted depending on context.

However, people get punished for having too many invalid issues as per CodeHawks docs. This means that whenever a sponsors communicates or sets a restraint, you are expected not to report the restraint as a valid issue because it could invalidate your correct ones. There's a reason why people don't report known issues. // I would agree with this if everything will be clear since the beginning within its code or documentation, but then I feel there are a lot of contradictions still, and the proof is this discussion we are having. Specifically in this situation, you see the code and then you see they check address zero for everything except the winners, you then check the documentation prepared that is not very descriptive in this way or clear and then you are expected to dig tons and tons of assumptions in chat/conversations etc for having a clarification. Thats why I recalled on code (which honestly should be their ultimate goal), and if we are assumming they want to use smart contracts capabilities and they miss one single check that can provoke something unexpected I believe its a valid issue.

I do agree that in platforms like Immumefi, code should be code and that's it. If you find a bug, it's a bug.// Yes agreed.

Thanks for your feedback!

[0xTenma]

1. Its more likely a validation issue. Trusting organizers doesn't mean we should allow to pass a zero address in the array of winners and ultimately lose the funds to zero address. This issue doesn't address something that can be exploited by a malicious organizer (in this case trust will break).
2. Zero address validation is missing in an area where funds can be lost that's why I guess judge marked it as HIGH severity.

Note: Zero address validation can't be compared with organizers putting wrong winners address in the array.

[quentin-abei]

I beg codehawks to introduce penality on any escalation made, i mean money penality. If one does an escalation on an issue he haven't submitted based purely on assumptions without providing a PoC that unvalidate the issue, hé must be punished. We can't tolerate so many comments on a single escalation that are purely based on assumptions. This waste judge time for nothing, and remember judges are working for free atm, so it should be a no-go zone for escalation based on assumptions.

This issue have already been proven valid by the original escalator, any more comments should provide a pure PoC and not assumptions. We are not doing politics here.

[ShaheenRehman]

Appeal!

This is a low issue. It's a user mistake issue which should be considered as a Low severity issue.

[romeroadrian]

Escalate

Overinflated severity.

[0xcats]

[APPEAL] I thought a lot about why the protocol has the distributeByOwner function. It seems that the distributeByOwner function is exactly for this case. According to the selected report the funds can be locked at the contract balance. Let's look at the distributeByOwner function:

196     * @notice Owner can rescue funds if token is stuck after the deployment and contest is over for a while

205    function distributeByOwner(
206        address proxy,
207        address organizer,
208        bytes32 contestId,
209        address implementation,
210        bytes calldata data
211    ) public onlyOwner {

217        _distribute(proxy, data);
218    }

It is obvious that the owner can send a new data to the _distribute. So even if the funds were not distributed by the deployProxyAndDistribute function they still can be distributed by the distributeByOwner function. So the severity of the lack zero check issue is Low/Informational.

You have completely missed the point of this issue and its duplicates, it is not about funds getting stuck but getting lost if sent to Address(0) since there are tokens that do not revert on transfer to addr0 but pass.

[ShaheenRehman]

Which is a user mistake. Still low

[0xhahax0]

User mistakes are not a valid finding, @PatrickAlphaC clarified this when he closed this finding: https://github.com/Cyfrin/2023-08-sparkn/issues/769#issuecomment-1709508206

This is a user mistake as well so it should be closed. Not sure where the inconsistency is coming from.

[0xcats]

Organizer being trusted was specified in the discord after readme was already released without that information, as agreed upon by CodeHawks. There are auditors who are not active in the discord chat and could not have known that now Organizer is suddenly trusted.

Other issues regarding malicious organizer able to distribute earnings to arbitrary address are also marked as valid.

If that is the case regarding the judge’s decision, then in this issue the Organizer can maliciously distribute to Addr(0), so, not user-mistake.

[ShaheenRehman]

User mistakes are not a valid finding, @PatrickAlphaC clarified this when he closed this finding: https://github.com/Cyfrin/2023-08-sparkn/issues/769#issuecomment-1709508206

This is a user mistake as well so it should be closed. Not sure where the inconsistency is coming from.

Prev I thought this is a low. But after reading Patrick's comment, I can say that this should be invalid not even low. Thanks @0xhahax0 for pointing out

[0xhahax0]

Organizer being trusted was specified in the discord after readme was already released without that information, as agreed upon by CodeHawks. There are auditors who are not active in the discord chat and could not have known that now Organizer is suddenly trusted.

Other issues regarding malicious organizer able to distribute earnings to arbitrary address are also marked as valid.

If that is the case regarding the judge’s decision, then in this issue the Organizer can maliciously distribute to Addr(0), so, not user-mistake.

Unfortunately this decision has been made after the SparkN contest has finished, in a discord channel, without an announcement. Following your logic - this decision should not be valid because not every auditor is active on Discord.. right?

There was a already a precedent set as explained in https://github.com/Cyfrin/2023-08-sparkn/issues/506#issuecomment-1711098870

Cyrfin team-members, sponsors and precedent all point to communication during the SparkN contest being part of the audit. It is what it is.

I do agree that it's good to know that for all contests from now on, the specs will be the final judgement. I hope the team makes an announcement via official channels because 'not all auditors use Discord', lol.

[PatrickAlphaC]

I accidentally had this marked as high. Thank you for the appeals. Looking at the invalidation notes now.

[PatrickAlphaC]

Going to leave it as low, as organizers are not necessarily trusted. Moving forward, zero address check and any "user input error" are likely going to be invalidated. This has been a great conversation.

Looking forward to people making a PR to the docs to implement/adjust rules as they see fit.

[PatrickAlphaC]

There might be an assumption that DAI will also revert, and it does not for address(0). So, yes, leaving as low.

Let's submit a finding to adjust our rules on this.