Impossible Finance

In the `IFFixedSale` contract, both the `withdrawGiveaway()` and `withdrawGiveawayVested()` functions accept an `allocation` input that can be specified by the user. In situations where the giveaway is not whitelisted, users can enter any `allocation` value without restrictions.

The `IFSale.setWithdrawDelay()` function allows the owner to set the withdrawal delay, and it calls `setWithdrawTime(endTime - withdrawDelay + _withdrawDelay)` to set the withdrawal time. The formula `endTime - withdrawDelay + _withdrawDelay` would be adequate for its initial setting, as the default value for `withdrawDelay` is zero. However, if the function is called a second time to adjust the withdrawal delay, the formula will incorrectly set the withdrawal time. In the worst case, the withdrawal time could be set earlier than the end time. The incorrect withdrawal time will impact the accuracy of the `IFVestable.getUnlockedToken()` function. This function depends on the `withdrawTime`, which may be incorrectly set. Therefore, if the withdrawal time ends up being earlier than it should be, it could result in more tokens vested than expected.

This is a consolidation of all the best practices we think could help improve the code quality. **Input Validation**: 1. `IFWhitelistable.setWhitelistSetter()`should have validation against zero-address as the parameter. Redundant code: 1. In `IFPurchasable._purchaseWithCode()`, the logic `if (paymentReceivedWithEachCode[_msgSender()][code] == 0 && paymentAmount > 0)` can be simplified to `if (paymentReceivedWithEachCode[_msgSender()][code] == 0)` because `paymentAmount > 0` is already checked for in the call to `_purchase()` few lines above. 2. The check `if (bytes(code).length > 0)` in `IFPurchasable._purchaseWithCode()#L178` is unnecessary, as the same check is performed in `L158` too. <br> **Gas optimization**: 1. In `IFSale.withdraw()` and `IFSale.withdrawGiveaway()`, the statement `require(tokenOwed != 0, 'withdraw giveaway amount 0');` can be moved earlier to save on gas if the execution reverts. 2. For loops can be gas-optimized by cashing the `array.length` in a `memory` variable and incrementing the iterator via `unchecked {++i;}` <br> **Code documentation**: 1. In `IFFixedSale.purchase(uint256)`, the revert statement is `revert("Use purchase(uint256 paymentAmount, bytes32[] calldata merkleProof, uint256 allocation)");`. However, `purchase(uint256 paymentAmount, bytes32[] calldata merkleProof, uint256 allocation)` does not exist. This is likely referring to `whitelistedPurchase(uint256 paymentAmount, bytes32[] calldata merkleProof, uint256 _allocation)` instead. A similar issue exists for `whitelistedPurchase(uint256, bytes32[] calldata)`. <br> **Code maintenance**: 1. Can replace `require(endTime + withdrawDelay < block.timestamp, 'cannot withdraw yet');` with the `onlyAfterSale` modifier for `IFFundable.cash()`. 2. Can use `EnumerableSet` to simplify the logic of having both `hasWithdrawn` and `withdrawerCount`. 3. In `IFFundable`, the `ONE_YEAR` and `FIVE_YEAR` values are leap year adjusted, while the `TEN_YEAR` value is not. Consider aligning it.

If the owner renounces their ownership, all ownable contracts will be left without an owner. Consequently, any function guarded by the `onlyOwner` modifier will no longer be able to be executed.

The `IFundable.cash()` function is designed to enable the project team to withdraw the payment tokens and any unsold sale tokens. However, in the context of a giveaway, where the `salePrice` is set to zero, this results in a failure due to the `IFSale.getSaleTokensSold()` function attempting to divide by `salePrice`, causing the `cash()` function to revert in giveaway scenarios. While payment tokens can still be withdrawn using `cashPaymentToken()` or `emergencyTokenRetrieve()`, any remaining sale tokens will be locked post-giveaway. It should be noted that the project team can utilize the `setWhitelist()` function to modify the whitelist, facilitating a "withdrawal" through the standard giveaway process by updating the Merkel root. However such manual intervention can introduce unnecessary risk due to human errors.

When `salePrice` is zero, a user should claim its token from `withdrawGiveaway()` (or related functions) instead of calling `purchase()`. There is no explicit check that when a user calls purchase, say in `IFSale` and `IFFixedSale`, the `salePrice` is non-zero. It is noted that the execution will revert implicitly due to a division by zero error in ` uint256 tokenPurchased = (paymentReceived[_msgSender()] * SALE_PRICE_DECIMALS) / salePrice;`, but this non-zero price check should be the responsibility of `IFPurchasable` rather than `IFSale`.

Every Solidity file specifies in the header a version number of the format `pragma solidity (^)0.8.9`. The caret (`^`) before the version number implies an unlocked pragma, meaning that the compiler will use the specified version _and above_, hence the term "unlocked". Furthermore, the current version used is `0.8.9` (as defined in `hardhat.config.ts`. It is recommended to use a newer version of the solidity compiler as mentioned [here](https://github.com/crytic/slither/wiki/Detector-Documentation#recommendation-80).

In the `IFFixedSale` contract, both `whitelistedPurchaseWithCode()` and `whitelistedPurchase()` functions determine whether the allocation should use the `publicAllocation` or the whitelisted allocation, depending on whether the `merkleProof` is provided. However, if a user's whitelisted amount is lower than the `publicAllocation`, the user can opt for the public allocation instead and receive more than what is allocated for whitelisted users. We consider the risk low, as we believe the project team is likely to grant larger allocations to whitelisted users. Nonetheless, we want to highlight this potential risk.