DeXe Protocol security record

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.

According to the design of the library `TraderPoolDivest`, the `_checkUserBalance()` function verifies whether the LP token amounts to be divested are less than the user's LP token balance (minus the amount of LP tokens received through investment in the current block) to prevent users from investing and divesting tokens in the same block. ```solidity=133 require( amountLP <= traderPool.balanceOf(msg.sender) - traderPool.investsInBlocks(msg.sender, block.number), "TP: wrong amount" ); ``` However, if the traderPool's LP token can be borrowed through a flashloan, the value of `traderPool.balanceOf(msg.sender)` may be increased to an arbitrary value, which can bypass the `_checkUserBalance()` function.

The function `canParticipate()` checks if the `voter` has sufficient balance to participate in voting. However, the value of ` ERC20(tokenAddress).balanceOf(voter)` in the `tokenBalance()` function can be manipulated by the `voter` through a flashloan if `isMicropool` is set to `false`. The `voter` can take out a flash loan and use the borrowed funds to increase the `tokenBalance()`, then the following check can be easily bypassed: ```solidity=547 (uint256 tokens, ) = tokenBalance(voter, isMicropool, useDelegated); if (tokens >= requiredVotes) { return true; } ``` ```solidity=404 function tokenBalance( address voter, bool isMicropool, bool useDelegated ) public view override returns (uint256 totalBalance, uint256 ownedBalance) { if (tokenAddress == address(0)) { return (0, 0); } totalBalance = _getBalanceInfoStorage(voter, isMicropool).tokenBalance; if (!isMicropool) { if (useDelegated) { ... } ownedBalance = ERC20(tokenAddress).balanceOf(voter).to18( ERC20(tokenAddress).decimals() ); totalBalance += ownedBalance; } } ``` This allows the user to create a proposal when they may not be able to.

According to the design of the `TraderPoolInvestProposal` contract, the purpose of the divest() function is to transfer profits from the specified investment proposal to the main pool and users. Upon calculating the rewards, the variable `claim[0]` - the amount of baseToken to be rewarded - is subtracted from both the `_proposalInfos[proposalId].lpLocked` and `totalLockedLP`. However, these two variables serve as trackers for changes in the trader pool's LP token. The function `min()` is utilized to prevent underflow errors, given that the reward base token could potentially exceed the invested base token. However, these two public variables may not be accurate. ```solidity=144 _proposalInfos[proposalId].lpLocked -= claimed[0].min( _proposalInfos[proposalId].lpLocked ); _updateFromData(user, proposalId, claimed[0]); investedBase -= claimed[0].min(investedBase); totalLockedLP -= claimed[0].min(totalLockedLP); ```

In the contract `GovUserKeeper`, the function `delegateTokens()` and `delegateNfts()` allow a delegator to delegate a certain amount of tokens or Nfts to a specified `delegatee`. The functions `undelegateTokens()` and `undelegateNfts()` can undelegate tokens or Nfts from a `delegatee`. However, a delegatee is able to call the function `createProposal()` in the contract `GovPool` to create a proposal. By voting on the created proposal, the `delegatee` updates the value of `micropoolInfo.maxTokensLocked`. Consequently, the delegators are unable to undelegate their tokens and NFTs since the following condition cannot be true: ```solidity=149 uint256 availableAmount = micropoolInfo.tokenBalance - micropoolInfo.maxTokensLocked; require( amount <= delegated && amount <= availableAmount, "GovUK: amount exceeds delegation" ); ```