Lack of check on _globalTaxRate in TokenSale::initialize and TokenSale::setAllocationAndTax results in excesive fees

Summary

The TokenSale::initialize function does not restrict the maximum value of globalTaxRate. this could result in excesive buying fees, making possible scenarios where users pays more taxes than token buys when using deposit function.
For example, user paying 10000000000000000000 usdc in taxes to buy 1000000000000000000 usdc token amounts, ie, paying 10x taxes.

Vulnerability Detail

The vulnerability exists in TokenSale::initialize and in TokenSale::setAllocationAndTax because it doesnt limit the value of _globalTaxRate

    function initialize(
        //...snippet
        uint256 _globalTaxRate,
        bool _isKYC,
        uint256 _whitelistTxRate
    ) external initializer {
        console.log("TokenSale@initialize _maxAllocation ",_maxAllocation);
        params = _params;
        stakingContract = IStaking(_stakingContract);
        //...snippet
        maxAllocation = _maxAllocation;
        globalTaxRate = _globalTaxRate;     //<@ no check for max value
        //...snippet
    }

    function setAllocationAndTax(uint256[3] calldata _allocations) external {
        //...snippet
        globalTaxRate = _allocations[1];
        whitelistTxRate = _allocations[2];
    }

So, globalTaxRate is used to calculate the taxfee for the users

function userTaxRate(
    uint256 _amount,
    address _sender
) public returns (uint256) {
    //... snippet
    if (_amount > userTaxFreeAllc) {
        if (isWhitelisted[_sender]) {
            return whitelistTxRate;
        } else {
            return globalTaxRate;
        }
    }
    //... snippet
}

And this is called in deposit, when users buys tokens

function deposit(uint256 _amount) external {
    //... snippet
    address sender = msg.sender;
    require(_amount > 0, "TokenSale: 0 deposit");
    //... snippet
    if (epoch == Epoch.Private) {
            _processPrivate(sender, _amount);
    }
}

function _processPrivate(address _sender, uint256 _amount) internal {
    //... snippet
    if (sum > taxFreeAllcOfUser) {
        uint256 userTxRate = userTaxRate(sum, _sender); // <@userTaxRate call
        if (s.amount < taxFreeAllcOfUser) {
            //... snippet
        } else {
            userTaxAmount = (amount * userTxRate) / POINT_BASE;
        }
    }
    //... snippet
    if (userTaxAmount > 0) {
        s.taxAmount += userTaxAmount;
        usdc.safeTransferFrom(_sender, marketingWallet, userTaxAmount);
    }
    //... snippet
}

So there is no limit for the taxFactor making it possible to charge user an upper arbitrary value, this userTaxAmount will be withdrawed from usdc user's balance. To show the impact, the following pool will charge 10x tax the amount buyed

//Modified from scripts/create_pool.js
const now = (await time.latest()).toNumber();
const newNow = BigInt(now)+BigInt(60*5);
const end = BigInt(now)+BigInt(60*15);

defaultParams = {
    totalSupply: BigInt("1000000000000000000"), //10e18
    token: defaultToken.address,
    privateTokenPrice: BigInt("1000000000000000000"),  //10e18
    initial: owner,
    privateStart: (newNow),
    privateEnd: (end),
    vestingPoints: [
      [end+BigInt(3*60), BigInt("500")],
      [end+BigInt(6*60), BigInt("300")],
      [end+BigInt(9*60), BigInt("200")],
    ],
};
/*This creates a new TokenSale contract and initializes
function createPoolNew(
    ITokenSale.Params memory _params,
    uint256 _maxAllocation,
    uint256 _globalTaxRate,
    bool _isKYC,
    uint256 _whitelistTxRate
)
*/
var tx_createp1 = await adminContract.createPoolNew(
    defaultParams,
    100000,
    10000,
    false,
    10000
);
var receipt1 = await tx_createp1.wait();
var poolAddr1 = receipt1.logs[1].args[0];
console.log("Created pool1 addr ",poolAddr1);

tokenSaleContract2 = await TokenSale.attach(poolAddr1);
// warp
await time.increaseTo(newNow);
await tokenSaleContract2.deposit(1);

Impact

The impact of this vulnerability includes:

Withdrawing arbitrary user's usdc amount by setting high tax fee using globalTaxRate variable
Loss of confidence in zap contracts

Code Snippet

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

Tool used

Manual Review

Recommendation

To mitigate this vulnerability it is recommended to set an upper limit in globalTaxRate variable

sherlock-audit / 2024-03-zap-protocol-judging

cryptonoob - Lack of check on _globalTaxRate in TokenSale::initialize and TokenSale::setAllocationAndTax results in excesive fees #128