DeXe Audit

According to the design of `GovPool`, the creator of the gov pool has the option to set the `earlyCompletion` parameter to true and the `validatorsVote` parameter to false, indicating that voting will complete as soon as the quorum is achieved without an additional validator step. However, this option can make the proposal vulnerable to flashloan attacks, where someone can instantly borrow large amounts of tokens to reach the quorum and execute the proposal. ```solidity=387 if (core.settings.earlyCompletion || voteEnd < block.timestamp) { if (_quorumReached(core)) { if (core.settings.validatorsVote && _govValidators.validatorsCount() > 0) { ... } else { return ProposalState.Succeeded; } } ``` Additionally, according to the design of the `GovPoolCreate` library, the function determines the type of proposal based solely on the value of `executors[executors.length - 1]`. If the value matches the address of `GovSetting`, `GovUserKeeper`, or `GovPool` contract, the proposal is considered internal. The `_handleDataForInternalProposal()` function then checks whether the proposal's `data` is legitimate. For instance, only the `setERC20Address()` and `setERC721Address()` functions in the `GovUserKeeper` contract can be executed through proposals. However, an attacker can change the value of `executors[executors.length - 1]` to any other contract and place the address of `GovUserKeeper` contract in another position. As a result, the `_handleDataForInternalProposal()` will be bypassed, allowing the attacker to perform other dangerous operations in the `GovUserKeeper` contract, such as `unlockToken()` and `withdrawToken()`, to steal the deposited tokens from other users. ```solidity=118 function _validateProposal( address[] calldata executors, uint256[] calldata values, bytes[] calldata data ){ ... address mainExecutor = executors[executors.length - 1]; settingsId = IGovSettings(govSettings).executorToSettings(mainExecutor); ... if (settingsId == uint256(IGovSettings.ExecutorType.INTERNAL)) { _handleDataForInternalProposal(govSettings, executors, values, data); } else if (...){ ... } ... } function __handleDataForInternalProposal(){ ... require( values[i] == 0 && executorSettings == uint256(IGovSettings.ExecutorType.INTERNAL) && (selector == IGovSettings.addSettings.selector || selector == IGovSettings.editSettings.selector || selector == IGovSettings.changeExecutors.selector || selector == IGovUserKeeper.setERC20Address.selector || selector == IGovUserKeeper.setERC721Address.selector || selector == IGovPool.editDescriptionURL.selector || selector == IGovPool.setNftMultiplierAddress.selector || selector == IGovPool.changeVerifier.selector), "Gov: invalid internal data" ); ... } ```

According to the logic of `TraderPoolExchange`, the `exchange()` function facilitates the exchange of `from` token to `to` token in the trader pool. Input validation is performed to ensure that the `from` token and `to` token are not blacklisted. ```solidity=43 require( !coreProperties.isBlacklistedToken(from) && !coreProperties.isBlacklistedToken(to), "TP: blacklisted token" ); ``` ```solidity=79 if (from != poolParameters.baseToken && from.thisBalance() == 0) { positions.remove(from); emit PositionClosed(from); } ``` However, if the `coreProperties` contract adds new blacklisted tokens via the `addBlacklistTokens()` function after an exchange operation, traders will be unable to close blacklisted position tokens since the remove operation in line 80 will never be reached. Consequently, the condition `traderPool.openPositions().length == 0` will never be true, and traders will be unable to perform divest operations in the `divest()` function of the `TraderPoolDivest` library. ```solidity=40 require(!senderTrader || traderPool.openPositions().length == 0, "TP: can't divest"); ```

Performing integer division before multiplication truncates the low bits, losing the precision of calculation. ## Function `buy()`: The linked code implements the calculation: $\text{saleTokenAmount} = \text{amount} * \text{exchangeRate} / \text{PRECISION}$. e.g. Suppose - amount = 15 - exchangeRate = 1/2 * PRECISION - tierView.vestingSettings.vestingPercentage = 2/10 * PERCENTAGE_100 We have `saleTokenAmount = 15 * 1/2 * PRECISION / PRECISION = 7`. Then `saleTokenAmount.percentage(PERCENTAGE_100 - tierView.vestingSettings.vestingPercentage) = 7 * (PERCENTAGE_100 - 2/10 * PERCENTAGE_100)/PERCENTAGE_100 = 5`. If the function applies multiplication before division, then the aforementioned result will be `6` instead of `5`. ## Function `_countPrefixVestingAmount()`: If `vestingSettings.unlockStep` is not a divisor of `vestingSettings.vestingDuration`, `purchase.vestingTotalAmount` may never be reached. e.g. Suppose - unlockStep == 3 - vestingDuration == 10 - vestingTotalAmount == 10 We have `stepsCount == vestingDuration / unlockStep = 10/3 = 3` and `tokensPerStep == vestingTotalAmount / stepsCount = 10/3 = 3` Then the maximum return value of `_countPrefixVestingAmount()` is `(vestingDuration / unlockStep) * tokensPerStep = (10 / 3) * 3 = 3 * 3 = 9 < vestingTotalAmount`.

According to the entire protocol's design, most internal calculations utilize 18 decimal places for tokens, with the exception of the TokenSaleProposal contract. In particular, the exchange rate between `tokenToBuyWith` and `saleToken`, as well as the transfer operations within this contract, may handle tokens incorrectly.

If the current time `block.timestamp` is before the `vestingView.cliffEndTime`, the `nextUnlockTime` is calculated as ```solidity=257 vestingView.nextUnlockTime = purchase.purchaseTime + uint64(uint256(vestingSettings.cliffPeriod).max(vestingSettings.unlockStep)); ``` However, `unlockStep` represents the tick step with which vested funds are given to the buyer, so it should instead be `purchase.purchaseTime + uint64(uint256(vestingSettings.cliffPeriod).max(vestingSettings.vestingDuration / vestingSettings.unlockStep))`. Similarly for the case when the current time `block.timestamp` is at least `vestingView.cliffEndTime`, `vestingSettings.unlockStep` should be replaced with `vestingSettings.vestingDuration / vestingSettings.unlockStep`. Another issue is that even with the above changes, the calculation of the unlock time can still be incorrect due to integer truncation. Let's suppose all mentions of `unlockStep` are replaced with `vestingSettings.vestingDuration / vestingSettings.unlockStep`. If `unlockStep` is not a divisor of `vestingDuration`, then it is possible for `nextUnlockTime` to be incorrect. The following example goes through the calculations shown in the code snippet: ```solidity=261 vestingView.nextUnlockTime = uint64(block.timestamp) + vestingSettings.unlockStep; vestingView.nextUnlockTime -= (vestingView.nextUnlockTime - purchase.purchaseTime) % vestingSettings.unlockStep; if (vestingView.nextUnlockTime > vestingView.vestingEndTime) { vestingView.nextUnlockTime = 0; } ``` e.g. Suppose - purchaseTime == 0 - vestingDuration == 10 - unlockStep == 3 - no cliff period - block.timestamp == 9 Then we have `vestingEndTime == 10`. In the calculation of `nextUnlockTime`, we have `nextUnlockTime == block.timestamp + vestingDuration / unlockStep = 9 + 10/3 = 12`. `nextUnlockTime -= (nextUnlockTime - purchaseTime) % (vestingDuration / unlockStep) = 12 % 3 = 0`. So `nextUnlockTime` is unchanged (still 12). As `nextUnlockTime == 12 > 10 == vestingEndTime`, we change `nextUnlockTime` to 0, when it should actually be 10.

There is no storage gap preserved in the logic contract. Any logic contract that acts as a base contract that needs to be inherited by other upgradeable child should have a reasonable size of storage gap preserved for the new state variable introduced by the future upgrades.

The `UserRegistry` contract stores the hash of the privacy policy in the variable `documentHash`, which users must agree to by signing the `Agreement` struct (using EIP712) to verify their agreement. Once the signature is verified, its hash is stored in `userInfo[msg.sender].signatureHash`. The view function `agreed()` can determine if a user has agreed to the policy by checking if `userInfor[msg.sender].signatureHash` is non-zero. ```solidity=53 function agreed(address user) external view override returns (bool) { return userInfos[user].signatureHash != 0; } function setPrivacyPolicyDocumentHash(bytes32 hash) external override onlyOwner { documentHash = hash; } ``` However, there is no validation to confirm the relationship between `signatureHash` and the current `documentHash`. The owner of the contract can change the `documentHash` using the function `setPrivacyPolicyDocumentHash()`, meaning that users who agreed to the previous policy will also agree to updates made to the policy.

The linked code allows `DistributionProposal` contract to transfer a certain amount of `rewardToken` tokens from `govAddress` to this contract. However, `approve()` operation is missing to grant approval to `DistributionProposal` contract. Without sufficient allowance, `DistributionProposal` contract is unable to transfer `rewardToken` tokens from `govAddress` account. ```solidity=75 if (address(rewardToken) == ETHEREUM_ADDRESS) { (bool status, ) = payable(voter).call{value: reward}(""); require(status, "DP: failed to send eth"); } else { if (balance < reward) { rewardToken.safeTransferFrom(govAddress, address(this), reward - balance); } rewardToken.safeTransfer(voter, reward); ```

According to the design of the `GovValidators` contract, the `createExternalProposal` contract receives the proposal from the `GovPool` contract, storing it in `_externalProposals[proposalId]` for voting. Once approved, the proposal can be executed through the `execute()` function in the `GovPool` contract. However, the `execute()` function does not update the state of `executed` in `_externalProposals[proposalId]`, which can lead to inaccuracies in the `getProposalState()` function. ```solidity=122 _externalProposals[proposalId] = ExternalProposal({ core: ProposalCore({ voteEnd: uint64(block.timestamp + duration), executed: false, quorum: quorum, votesFor: 0, snapshotId: uint32(govValidatorsToken.snapshot()) }) }); ```

According to the design of the `TraderPoolInvestProposal` contract, the `convertInvestedBaseToDividend()` function is to convert newly invested funds to rewards. Reward distribution of the base token can be very different depending on when `convertInvestedBaseToDividends()` is called. For example, suppose userA is the first user to invest base tokens and userB is the second user to invest base tokens. Case 1: `convertInvestedBaseToDividends()` is called after userA but before userB - userA's investment will be given as rewards only to userA - If `convertInvestedBaseToDividends()` is called after userB invests, userB's investment is given to both userA and userB as rewards Case 2: `convertInvestedBaseToDividends()` is called after both users invest and not before - userA's investment and userB's investment will be given as rewards to both userA and userB So the comparison of the two is that userA gets more rewards in Case 1 and less in Case 2 while the opposite is true for userB.