ZdravkoHr. - Blocklisted investors can still claim USDC in `TokenSale.sol`

[sherlock-admin3]

ZdravkoHr.

high

Blocklisted investors can still claim USDC in TokenSale.sol

Summary

A wrong argument is passed when checking if a user is blacklisted for claiming in TokenSale.claim(). Because the check is insufficient, blocked users can claim their USDC.

Vulnerability Detail

Admin.setClaimBlock() blocks users from claiming. The function accepts the address of the user to be blocked and adds it to the blockClaim mapping.

    /**
     @dev Whitelist users
     @param _address Address of User
     */
    function setClaimBlock(address _address) external onlyRole(OPERATOR) {
        blockClaim[_address] = true;
    }

The check in Admin.claim() wrongly passes address(this) as argument when calling Admin.blockClaim.

        require(
            uint8(epoch) > 1 && !admin.blockClaim(address(this)),
            "TokenSale: Not time or not allowed"
        );

In this context, address(this) will be the address of the token sale contract and the require statement can be bypassed even by a blocked user.

Impact

The whole functionality for blocking claims doesn't work properly.

Code Snippet

    function claim() external {
        checkingEpoch();
        require(
            uint8(epoch) > 1 && !admin.blockClaim(address(this)),
            "TokenSale: Not time or not allowed"
        );

        Staked storage s = stakes[msg.sender];
        require(s.amount != 0, "TokenSale: No Deposit");
        require(!s.claimed, "TokenSale: Already Claimed");

        uint256 left;
        (s.share, left) = _claim(s);
        require(left > 0, "TokenSale: Nothing to claim");
        uint256 refundTaxAmount;
        if (s.taxAmount > 0) {
            uint256 tax = userTaxRate(s.amount, msg.sender);
            uint256 taxFreeAllc = _maxTaxfreeAllocation(msg.sender) * PCT_BASE;
            if (taxFreeAllc >= s.share) {
                refundTaxAmount = s.taxAmount;
            } else {
                refundTaxAmount = (left * tax) / POINT_BASE;
            }
            usdc.safeTransferFrom(marketingWallet, msg.sender, refundTaxAmount);
        }
        s.claimed = true;
        usdc.safeTransfer(msg.sender, left);
        emit Claim(msg.sender, left);
    }

Tool used

Manual Review

Recommendation

Pass the address of the user.

        require(
-            uint8(epoch) > 1 && !admin.blockClaim(address(this)),
+            uint8(epoch) > 1 && !admin.blockClaim(msg.sender)),
            "TokenSale: Not time or not allowed"
        );

[0502lian]

BlockClaim function is used for instance(block the whole tokeSale ), not for one user.

[Coareal]

Escalate

Issue is invalid. Implementation is correct and intended. The mentioned check is used to block a specific tokenSale instance, and not that of a user.

[sherlock-admin2]

Escalate

Issue is invalid. Implementation is correct and intended. The mentioned check is used to block a specific tokenSale instance, and not that of a user.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[s1ce]

Issue should be valid. Comments in the code seem to suggest that this is on a per user basis.

[ZdravkoHr]

Agree with @s1ce. According to the Sherlock hierarchy of truth protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

[0xDenzi]

Issue should be invalid, the only issue here is that the comments are wrong.

[Hash01011122]

Can you give any reason on why this issue should be invalidated?? @Coareal @omar-ahsan

[0xDenzi]

@Hash01011122

    /**
     @dev Whitelist users
     @param _address Address of User
     */
    function setClaimBlock(address _address) external onlyRole(OPERATOR) {
        blockClaim[_address] = true;
    }

The comments above the function indicate whitelisting of users but this function is not intended to whitelist any address. setClaimBlock() as the name suggests is used to block an address by setting it to true in blockClaim. Similarly

The comments by sponsor team indicate that this function is used to block all incoming claims for a particular Token Sale which means all users can not claim from the token sale during the blocked duration. Currently the function does as intended according to this description.

Further more the code already contains a function to blacklist users i.e addToBlackList(). This function performs the blocking of single users by blocking the deposit() function for blacklisted users which is the entry point to the token sale.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Lithium-Ventures/zap-contracts-labs/pull/3

[Hash01011122]

Glad @omar-ahsan you did point out this mistake. My question to you is were watsons aware of this at the time of contest if not this is a valid finding.

[0xDenzi]

Glad @omar-ahsan you did point out this mistake. My question to you is were watsons aware of this at the time of contest if not this is a valid finding.

Since the comments are misleading and not correct, the next source of information was the dev in the public chat. The screenshot of the message in my previous reply is from the public chat hence everyone knew this information.

[Evert0x]

Planning to accept escalation and invalidate issue

[ZdravkoHr]

@Evert0x, so the Sherlock documentation should be updated to discord > comments?

[Hash01011122]

@Evert0x I think this is a valid issue, as Watson's were not aware of it before or at the time of the contest. @omar-ahsan I understand that you had conversation with sponsors and they responded you in discord but not every watson was aware of it. I would like @Evert0x to reconsider his decision.

[Evert0x]

@Hash01011122 do you know if there was any other language for this function in the code or docs?

[Hash01011122]

As far as I know there were no mentions about this in docs or in codebase

[detectiveking123]

@Evert0x @Czar102

There are a lot, and I mean a lot, of issues with the documentation in this codebase. This has left a lot of ambiguity in terms of what the sponsors actually want vs the design decisions they've consciously made.

I would recommend only rewarding issues that actually cause a loss of funds or very, very clearly break protocol functionality (i.e. it's just definitely not a design decision). This issue, as well as #87 and #56, fall into the category of "maybe the sponsors intended this, maybe they didn't", and I don't think any of them should be valid issues.

[Nilay27]

@Evert0x @Czar102

There are a lot, and I mean a lot, of issues with the documentation in this codebase. This has left a lot of ambiguity in terms of what the sponsors actually want vs the design decisions they've consciously made.

I would recommend only rewarding issues that actually cause a loss of funds or very, very clearly break protocol functionality (i.e. it's just definitely not a design decision). This issue, as well as #87 and #56, fall into the category of "maybe the sponsors intended this, maybe they didn't", and I don't think any of them should be valid issues.

@detectiveking123, I understand your concerns about potentially overreporting issues that may be interpreted as design decisions rather than genuine flaws.

However, as a Watson, we rely heavily on the documentation provided to guide our auditing process. When the documentation is unclear and the sponsors are not available for clarification, we must address potential vulnerabilities based on our best understanding of the intended functionality.

Considering the nature of a competitive audit, dismissing ambiguities that arise from unclear documentation could inadvertently overlook genuine issues and waste a lot of Watsons' time due to a lack of due diligence before the audit.

@Evert0x @Czar102 @Hash01011122, I would like to request that we only finalize these issues once after the sponsors' confirmation. While this might be a bit of a hassle, but this would be the fairest approach.

[Hash01011122]

I stand by what I mentioned earlier that this should remain a valid issue. @Evert0x @Czar102

[Evert0x]

With the hierarchy of truth at the time of the contest I believe the right judgment is to reject the escalation and keep the issue valid.

[detectiveking123]

@Evert0x Judgement doesn't make sense. By that logic, there are so many other issues in this contest that should be valid, just because the documentation is completely wrong.

[Hash01011122]

@detectiveking123 I've already justified the validity of this issue above. If you can provide a counterargument using any rule from Sherlock's documentation, please do so. If not please refrain to comment on this issue.

[Evert0x]

Result: Medium Has Duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• Coareal: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.