Silvermist - TokenSale.sol#calculateMaxAllocation() Incorrect return values

[sherlock-admin2]

Silvermist

medium

TokenSale.sol#calculateMaxAllocation() Incorrect return values

Summary

A user can have 0 allocations and get themaxAllocation or a user can exceed the maximum allocations.

Vulnerability Detail

maxAllocation is the max baseline a user can invest in a pool. However, swapped return values allow a user to have 0 allocations and get themaxAllocation or to exceed the maximum allocations.

    function calculateMaxAllocation(address _sender) public returns (uint256) {
        uint256 userMaxAllc = _maxTierAllc(_sender);

        if (userMaxAllc > maxAllocation) {
            return userMaxAllc;
        } else {
            return maxAllocation;
        }
    }

Impact

A user can have 0 allocations and get themaxAllocation or a user can exceed the maximum allocations

Code Snippet

https://github.com/sherlock-audit/2024-03-zap-protocol/blob/c2ad35aa844899fa24f6ed0cbfcf6c7e611b061a/zap-contracts-labs/contracts/TokenSale.sol#L259-L267

Tool used

Manual Review

Recommendation

    function calculateMaxAllocation(address _sender) public returns (uint256) {
        uint256 userMaxAllc = _maxTierAllc(_sender);

        if (userMaxAllc > maxAllocation) {
-            return userMaxAllc;
+            return maxAllocation;
        } else {
-            return maxAllocation;
+            return userMaxAllc;

        }
    }

Duplicate of #152

[ZdravkoHr]

Escalate

Since the escalation period is coming to an end and there were no sponsor comments at all, I am escalating this issue because it's not clear if it's indended or not

[sherlock-admin2]

Escalate

Since the escalation period is coming to an end and there were no sponsor comments at all, I am escalating this issue because it's not clear if it's indended or not

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.

[Hash01011122]

Had the same issue, sponsors weren't responding much so I was unable to confirm whether this was valid issue or design choice.
But on deeper inspection it appears maxAllocation is the default maxAllocation for all users,

[The-first-audit]

I believe this statement continuously increases user allocation even beyond maximum..

https://github.com/sherlock-audit/2024-03-zap-protocol/blob/c2ad35aa844899fa24f6ed0cbfcf6c7e611b061a/zap-contracts-labs/contracts/TokenSale.sol#L300

thus the reason why the if statement above to regulate the max allocation

[sherlock-admin4]

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

[s1ce]

@Hash01011122 @Czar102 Just noticed this but my issue #172 is incorrectly marked a duplicate of #152 when it is a duplicate of this issue instead. Honestly thought all of these were duplicates until I took a closer look.

[Evert0x]

@Hash01011122 Do I understand correctly that you want to make this report valid?

[Hash01011122]

@Evert0x Yeah this is a valid finding and sponsors have confirmed it too.

[Evert0x]

Planning to accept escalation and make issue family Medium

[s1ce]

@Evert0x Can you make sure to duplicate #172 with this? It is the same issue but was duplicated with something else incorrectly.

@Hash01011122 please confirm

[Evert0x]

@Hash01011122 revisiting the issue.

It's unclear what the protocol intention is with this function. It's one of the two below.

1. It's a sanity check, if userMaxAllc is higher than the global limit maxAllocation, make sure that everyone adheres to the global limit. This would make the issue valid.

2. It's a design choice to allow specific users to exceed the maxAllocation, this would make the issue invalid.

Is there any code comment / discord message that indicates the intention of the protocol team?

[Evert0x]

I believe it's clear that 2) is the case https://discord.com/channels/812037309376495636/1217849746845339678/1219806056625078313

Planning to reject escalation and keep issue invalid

[The-first-audit]

The protocol team fixed this issue in PR/commit https://github.com/Lithium-Ventures/zap-contracts-labs/pull/1.

I think if the protocol intended it to exceed max. They won’t fix it as stated above.

[Evert0x]

Result:
Invalid
Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZdravkoHr: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.