Validators can only be added but not removed

Handle pants Vulnerability details The contract allows only to add validators but doesn't allow to remove them. The addValidator fucntion adds them and there is no function to remove a Validator. This is ability the owner should have. A similar issue is here: code-423n4/2021-06-gro-findings51 ---...

takeOutRewardTokens(): epochs calculation should be rounded up

Handle hickuphh3 Vulnerability details Impact If the owner would like to remove rewards, the number of epochs affected could potentially be 1 less because solidity division rounds down, resulting in more rewards taken out than allowed. Proof of Concept Assume currentEpoch is 1000 end epoch is 200...

unstake should update exchange rates first

Handle cmichel Vulnerability details The unstake function does not immediately update the exchange rates. It first computes the validatorSharesRemove = tokensToSharesamount, v.exchangeRate with the old exchange rate. Only afterwards, it updates the exchange rates if the validator is not disabled:...

DOS By Front Running DelegatedStaking initialize

Handle elprofesor Vulnerability details Impact DelegatedStaking utilizes the ERC1967 upgradeable proxy standard. This relies on an implementation contract being deployed and then reused or consumed by a proxy contract. As proxy contracts are unable to leverage a constructor they typically use an...

MochiTreasuryV0.withdrawalCRV use unsafe transfer of ERC20

Handle pants Vulnerability details MochiTreasuryV0.withdrawalCRV use crv.transfer. There are some ERC20 tokens, as USDT for example, that aren't safe to use as crv. Since USDT.transfer doesn't revert on fail, the transfer may fail and because the return value is ignored the function will not...

Validator can fail to receive commission reward in redeemAllRewards

Handle jonah1005 Vulnerability details Impact Validator can fail to receive commission reward by calling redeemAllRewards. There's a check in redeemAllRewards uint128 rewards = sharesToTokenss.shares, v.exchangeRate - s.staked; requirerewards 0, "Nothing to redeem"; The validator's tx might be...

Usage of an incorrect version of Ownbale library can potentially malfunction all onlyOwner functions

Handle WatchPug Vulnerability details // this is used to have the contract upgradeable function initializeuint128 minStakedRequired public initializer Based on the context and comments in the code, the DelegatedStaking.sol contract is designed to be deployed as an upgradeable proxy contract...

Epoch may rounded to zero in deposit depositRewardTokens and takeOutRewardTokens

Handle jonah1005 Vulnerability details division bias in deposit depositRewardTokens and takeOutRewardTokens Impact When the owner deposits reward into the contract, the remainder would not be counted. These dust tokens would be left in the contract. There's a similar issue in takeOutRewardTokens...

unstake(): validatorSharesRemove should be calculated after updateValidator() is called

Handle hickuphh3 Vulnerability details Impact When unstaking from an enabled validator, the number of validator shares to remove should be calculated using the updated exchange rate. Otherwise, more validator shares are removed than required. Proof of Concept We utilise the example found in the...

exitTempusAMM can be made to fail

Handle cmichel Vulnerability details There's a griefing attack where an attacker can make any user transaction for TempusController.exitTempusAMM fail. In exitTempusAMM, the user exits their LP position and claims back yield and principal shares. The LP amounts to redeem are determined by the...

depositAndFix can be made to fail

Handle cmichel Vulnerability details There's a griefing attack where an attacker can make any user transaction for TempusController.depositAndFix fail. In depositAndFix, swapAmount many yield shares are swapped to principal where swapAmount is derived from the function arguments. A final...

Wrong implementation of CreditLimitByMedian.sol#getLockedAmount() makes it unable to unlock lockedAmount in CreditLimitByMedian model

Handle WatchPug Vulnerability details function getLockedAmount LockedInfo memory array, address account, uint256 amount, bool isIncrease public pure override returns uint256 if array.length == 0 return 0; uint256 newLockedAmount; if isIncrease ... else for uint256 i = 0; i amount newLockedAmount ...

Rebalance will fail due to low precision of percentages

Handle cmichel Vulnerability details The AssetManager.rebalance function has a check at the end to ensure that all tokens are deposited again: requiretoken.balanceOfaddressthis == 0, "AssetManager: there are remaining funds in the fund pool"; The idea is that the last market deposits all...

Wrong calculation of FrozenCoinAge

Handle WatchPug Vulnerability details The current implementation of Comptroller.soladdFrozenCoinAge and UserManager.solgetFrozenCoinAge, the duration of overdue time is calculated based on the block number of lastRepay and the current block number. However, since the borrower may have never repai...

Wrong implementation of CreditLimitByMedian.sol#getLockedAmount() will lock a much bigger total amount of staked tokens than expected

Handle WatchPug Vulnerability details function getLockedAmount LockedInfo memory array, address account, uint256 amount, bool isIncrease public pure override returns uint256 if array.length == 0 return 0; uint256 newLockedAmount; if isIncrease for uint256 i = 0; i arrayi.lockedAmount...

Comptroller rewards can be artificially inflated and drained by manipulating [totalStaked - totalFrozen] (or: wrong rewards calculation)

Handle kenzo Vulnerability details By adding a small of amount of staking to a normal user scenario, and not approving this small amount as a loan for anybody, a staker can gain disproportionate amounts of comptroller rewards, even to the point of draining the contract. For example: Stakers A,B,C...

borrow must accrueInterest first

Handle cmichel Vulnerability details The UToken.borrow function first checks the borrowed balance and the old credit limit before accruing the actual interest on the market: // @audit this uses the old value requireborrowBalanceViewmsg.sender + amount + fee = amount + fee, "UToken: The loan amoun...

Rebalance will fail if a market has high utilization

Handle cmichel Vulnerability details The AssetManager.rebalance function iterates through the markets and withdraws all tokens in the moneyMarketsi.withdrawAll call. Note that in peer-to-peer lending protocols like Compound/Aave the borrower takes the tokens from the supplier and it might not be...

AssetManager's rebalance function can fail if the last moneyMarket doesn't support a token and the balance gathered is odd

Handle hyh Vulnerability details Summary AssetManager's rebalance function, , will fail if the last moneyMarket doesn't support a token its supportsTokentokenAddress is false, while remaining balance be positive, which can be the case even if no allocation to the last moneyMarket is due as a dust...

debtWriteOff updates totalFrozen immaturely, thereby losing staker rewards

Handle kenzo Vulnerability details debtWriteOff updates totalFrozen before withdrawing unionToken rewards. As the borrower is overdue, this means the staker calling debtWriteOff will lose his rewards if for example totalStaked == totalFrozen. Note: If the borrower would to first call...

UnionToken should check whitelist on from?

Handle cmichel Vulnerability details The UnionToken can check for a whitelist on each transfer in beforeTokenTransfer: if whitelistEnabled requireisWhitelistedmsg.sender || to == address0, "Whitelistable: address not whitelisted"; This whitelist is checked on msg.sender not on from, the token...

exitTempusAMMAndRedeem redeems to the wrong account

Handle cmichel Vulnerability details In TempusController.exitTempusAMMAndRedeem the first one, the inner exitTempusAMMGivenAmountsOut call redeems LP tokens and sends the yield&principal shares to the msg.sender already. It then tries to redeem the received shares for backing tokens or...

UserManager: updateLockedData() locks more amount than required.

Handle itsmeSTYJ Vulnerability details Impact The function updateLockedData calls creditLimitModel.getLockedAmount which is executed in a for loop with amount passed as an argument. This means that all stakers that are staking on behalf of the borrower are asked to lock amount . If I wanted to lo...

UserManager: debtWriteOff() function doesn't update state in comptroller

Handle itsmeSTYJ Vulnerability details Thoughts The debtWriteOff function calls updateTotalFrozen instead of updateTotalFrozen. It is not clear if the comptroller's state should also be updated if totalStaked is modified which it is when you write debt off. If it is then updateTotalFrozen should ...

UserManager: totalStaked ≥ totalFrozen should be checked before and after totalFrozen is updated

Handle itsmeSTYJ Vulnerability details Impact The require statement in updateTotalFrozen and batchUpdateTotalFrozen to check that totalStaked ≥ totalFrozen should be done both before and after updateTotalFrozen is called to ensure that totalStake is still ≥ totalFrozen. This will serve as a sanit...

UserManager: updateLockedData() doesn't check that the amount is actually locked.

Handle itsmeSTYJ Vulnerability details Impact The function updateLockedData does not actually check if the amount required to be locked is actually locked. Proof of Concept Same solution as my other high issue. I've added comments where relevant. function updateLockedData address borrower, uint25...

Financial loss :: commissionAvailableToRedeem is assigned incorrectly

Handle csanuragjain Vulnerability details Impact This can lead to financial loss where validator will lose the commissionAvailableToRedeem Proof of Concept 1. Navigate to 2. Check the redeemRewards function 3. Let us consider the case where msg.sender == v.address ifmsg.sender == v.address...

Users could lose funds if owner took out reward which is not multiplier of allocatedTokensPerEpoch

Handle xYrYuYx Vulnerability details Impact User could lost funds if owner take reward which is not multiplier of allocatedTokensPerEpoch. Proof of Concept This is my test case to proof this issue. This issue occur because of Line 104 Ex. If allocatedTokensPerEpoch is 1 CQT. and now available epo...

Unable to redeem rest funds if deposited reward is not multiplier of allocatedTokensPerEpoch

Handle xYrYuYx Vulnerability details Impact Owner will deposit any amount of reward if amount is greater than allocatedTokensPerEpoch. This means that it is possible that owner can sent amount which is not multiplier of allocatedTokensPerEpoch. For example, when allocatedTokensPerEpoch is 1 CQT,...

Change in interest rate can disable repay of loan

Handle pmerkleplant Vulnerability details Impact The ability of a borrower to repay a loan is disabled if the interest rate is set too high by the InterestRateModel. However, there is neither a check when setting the interest rate nor an indication in the IInterestRateModel's specs of this...

reward tokens could get lost due to rounding down

Handle gpersoon Vulnerability details Impact The function depositRewardTokens divides the "amount" of tokens by allocatedTokensPerEpoch to calculate the endEpoch. When "amount" isn't a multiple of allocatedTokensPerEpoch the result of the division will be rounded down, effectively losing a number...

disableValidator() can distort totalGlobalShares (edge case)

Handle gpersoon Vulnerability details Impact Assume a validator has been added Assume this validator calls disableValidator maybe by accident or it recognizes a mistake Assume stake hasn't been called yet and endEpoch is still 0 The function disableValidator will execute and set v.disabledEpoch t...

allocatedTokensPerEpoch cannot be changed under special scenario

Handle csanuragjain Vulnerability details Impact allocatedTokensPerEpoch will fail to change and higher rewards would be given. Proof of Concept 1. Navigate to 2. Check the setAllocatedTokensPerEpoch function function setAllocatedTokensPerEpochuint128 amount public onlyOwner requireamount 0,...

Incorrect updateGlobalExchangeRate implementation

Handle xYrYuYx Vulnerability details Impact UpdateGlobalExchangeRate has incorrect implementation when totalGlobalShares is zero. If any user didn't start stake, totalGlobalShares is 0, and every stake it will increase. but there is possibility that totalGlobalShares can be 0 amount later by...

takeOutRewardTokens does not work correctly

Handle csanuragjain Vulnerability details Impact Owner will not be able to take out reward Proof of Concept 1. Navigate to 2. Check the takeOutRewardTokens function function takeOutRewardTokensuint128 amount public onlyOwner requireamount 0, "Amount is 0"; uint128 currentEpoch =...

Steal tokens from TempusController

Handle gpersoon Vulnerability details Impact The function depositAndProvideLiquidity can be used go retrieve arbitrary ERC20 tokens from the TempusController.sol contract. As the test contract of TempusController.sol shows, it has indeed ERC20 tokens. The problem is due to the fact that you suppl...

Signature replay attacks for different identities (nonce on wrong party)

Handle cmichel Vulnerability details A single QuickAccount can serve as the "privilege" for multiple identities, see the comment in QuickAccManager.sol: NOTE: a single accHash can control multiple identities, as long as those identities set it's hash in privilegesaddressthis. this is by design If...

QuickAccManager Smart Contract signature verification can be exploited

Handle cmichel Vulnerability details Several different signature modes can be used and Identity.execute forwards the signature parameter to the SignatureValidator library. The returned signer is then used for the privileges check: address signer = SignatureValidator.recoverAddrImplhash, signature...

QuickAccManager.sol#send() Potential replay attack

Handle WatchPug Vulnerability details In QuickAccManager.solsend, addressidentity is not included in the txHash, makes it possible to replay the transaction when the same QuickAccount accHash controls multiple Identity. function sendIdentity identity, QuickAccount calldata acc, DualSig calldata...

Incorrect checking of signature length

Handle JMukesh Vulnerability details Impact signature which have SignatureMode.EthSign/SignatureMode.EIP712 have length 65 , so all signature coming through both mode will be reverted Proof of Concept Tools Used manual review Recommended Mitigation Steps update the correct signature length --- Th...

QuickAccManager.sol Potential replay attack

Handle WatchPug Vulnerability details This issue is possibly duplicate with a previous issue named "Potential replay attack" In QuickAccManager.solsendTxns and QuickAccManager.solsendTransfer, addressidentity is not included in the txHash, makes it possible to replay the transaction on another...

cancel() calculates hashTx in the wrong way

Handle gpersoon Vulnerability details Impact The function cancel of contract QuickAccManager uses the wrong way to calculate the hash that has to be cancelled. It uses: "bytes32 hashTx = keccak256abi.encodeaddressthis, block.chainid, accHash, nonce, txns;" Where it should use "bytes32 hash =...

Prevent execution with invalid signatures

Handle gpersoon Vulnerability details Impact Suppose one of the supplied addrsi to the constructor of Identity.sol happens to be 0 by accident. In that case: privileges0 = 1 Now suppose you call execute with an invalid signature, then recoverAddrImpl will return a value of 0 and thus signer=0. If...

Duplicate utoken and usermanager can be added which cannot be deleted

Handle csanuragjain Vulnerability details Impact If Admin decides to delete the market, only the first instance of utoken and usermanager gets deleted. This means duplicate instance remains and Admin has actually not deleted the market Proof of Concept 1. Navigate to 2. Check the addUToken functi...

QuickAccManager.sol#cancel() Wrong hashTx makes it impossible to cancel a scheduled transaction

Handle WatchPug Vulnerability details In QuickAccManager.solcancel, the hashTx to identify the transaction to be canceled is wrong. The last parameter is missing. As a result, users will be unable to cancel a scheduled transaction. function cancelIdentity identity, QuickAccount calldata acc, uint...

User funds are lost in case of non supported market token deposit

Handle csanuragjain Vulnerability details Impact User funds can be lost as current logic cannot withdraw unsupported market token even though deposit can be done for same Proof of Concept 1. Navigate to 2. Check the function deposit function depositaddress token, uint256 amount external override...

MAX_TRUST_LIMIT might be too high

Handle gpersoon Vulnerability details Impact Both SumOfTrust.sol and CreditLimitByMedian.sol contain an expensive sort function. This is used by UserManager.sol via the functions getLockedAmount and getCreditLimit. If the list of stakers would be very long then the sort would take up all the gas...

rebalance function will fail due to invalid condition

Handle csanuragjain Vulnerability details Impact User will be unable to rebalance the fund Proof of Concept 1. Navigate to contract at 2. Check the rebalance function function rebalanceaddress tokenAddress, uint256 calldata percentages external override checkMarketSupportedtokenAddress onlyAdmin...

User Fund loss in case of Unsupported Market token deposit

Handle csanuragjain Vulnerability details Impact User funds can be lost as current logic cannot withdraw unsupported market token even though deposit can be done for same Proof of Concept 1. Navigate to 2. Check the function deposit function depositaddress token, uint256 amount external override...

PrizePool.awardExternalERC721() Erroneously Emits Events

Handle leastwood Vulnerability details Impact The awardExternalERC721 function uses solidity's try and catch statement to ensure a single tokenId cannot deny function execution. If the try statement fails, an ErrorAwardingExternalERC721 event is emitted with the relevant error, however, the faile...