[WP-H9] LenderPool.sol#start() startFeeFraction can be used by a malicious/compromised owner to rug lenders

Lines of code Vulnerability details A configurable startFeeFraction with no upper bound can be claimed by the caller to a specified address. The fee is not based on the gas cost, but on the totalLent of the pool. We believe this startFee reward is unnecessary and it creates a potential rug vector...

DoS: Attacker May Front-Run CoreFactory.createProject() With A _projectId Causing Future Transactions With The Same _projectId to Revert

Lines of code Vulnerability details Impact A projectId may only be used once in CoreFactory.createProject since the modifier onlyAvailableProject will revert if project.creator != 0. The result is an attacker may front-run any createProject transaction in the mem pool and create another...

Deposited collateral can't be withdrawn when PooledCreditLineStatus is Expired

Lines of code Vulnerability details Impact A user who deposits on an PooledCreditLineStatus.EXPIRED will not be able to withdraw the collateral Proof of Concept 1. Alice uses depositCollateral with 100 USDC on an Expired credit line - Here's the code 2. Alice attempts to use withdrawCollateral th...

Calling approve() without first calling approve(0) causes problems with non-standard tokens (e.g. USDT)

Lines of code Vulnerability details This is another instance of the same issue that was found in the last sublime contest. In that issue the judge upgraded the finding to a risk of Medium. Impact Calling approve without first calling approve0 will revert with some tokens, such as Tether USDT. Thi...

Gas costs will likely result in any fees sent to the Splitter being economically unviable to recover.

Lines of code Vulnerability details Impact Collection owners will likely lose money by claiming fees unless the fees from a single NFT sale outweighs the cost of claiming it not guaranteed. Proof of Concept Consider a new Collection with a RoyaltyVault and Splitter set and a nonzero mint fee. Whe...

CoreCollection's token transfer can be disabled

Lines of code Vulnerability details Impact When royaltyAsset is an ERC20 that doesn't allow zero amount transfers, the following griefing attack is possible, entirely disabling CoreCollection token transfer by precision degradation as both reward distribution and vault balance can be manipulated...

LenderPool: Principal withdrawable is incorrectly calculated if start() is invoked with non-zero start fee

Lines of code Vulnerability details Details & Impact The principalWithdrawable calculated will be more than expected if start is invoked with a non-zero start fee, because the borrow limit is reduced by the fee, resulting in totalSupplyid not being 1:1 with the borrow limit. function...

[WP-M7] Wrong design/implementation of interest accrued to the borrowers can make the lenders to end up getting much fewer amount of interest than expected

Lines of code Vulnerability details function calculateInterest uint256 principal, uint256 borrowRate, uint256 timeElapsed internal pure returns uint256 return principal.mulborrowRate.multimeElapsed.divYEARINSECONDS.divSCALINGFACTOR; function updateStateOnPrincipalChangeuint256 id, uint256...

[WP-H1] LenderPool.sol#terminate() Wrong value is used for the shares argument of SAVINGS_ACCOUNT.withdrawShares() can cause fund loss to all users

Lines of code Vulnerability details / @notice Function invoked when pooled credit line is terminated by admin @dev only pooledCreditLineContract can invoke @param id identifier for the pooled credit line @param to address to which all the borrow tokens are transfered / function terminateuint256 i...

_withdrawLiquidity() is Not Consistent With start()

Lines of code Vulnerability details Impact withdrawLiquidity details a number of scenarios under which a user may withdraw their liquidity. The first two scenarios outline cases where the credit line has either been cancelled by the borrower or insufficient assets have been lent out to the pool b...

Any User Can Register Them-self if signerAddress is Unset

Lines of code Vulnerability details Impact The twitter verifier contract will sign twitter accounts using some signerAddress. However, if for whatever reason this is unset or happens to be the zero address, then anyone could bypass the requiresigner == signerAddress, 'RS5'; check in registerSelf...

Use safeTransfer

Lines of code Vulnerability details Impact The return value of the transfer is not checked. You already imported SafeERC20 into this contract, so you can use it to resolve this issue. Proof of Concept Tools Used manual code review Recommended Mitigation Steps IERC20borrowAsset.safeTransferto, fee...

[WP-H11] lender may not be able to get back their funds, due to improper handling of potential loss of strategy

Lines of code Vulnerability details uint256 notBorrowed = pooledCLConstantsid.borrowLimit.subPOOLEDCREDITLINE.getPrincipalid; uint256 notBorrowedInShares = IYieldstrategy.getSharesForTokensnotBorrowed, borrowAsset; uint256 sharesHeld = pooledCLVariablesid.sharesHeld; requiresharesHeld != 0,...

Fee in start() can be avoided

Lines of code Vulnerability details Impact A fee is collected in start that does not get collected if the borrowLimit is reached in lend. Proof of Concept if a start gets called and the amount - fee minBorrowAmount then a fee gets collected before accept is called. if maxLent is met in lend then...

approve and safeApprove Should Approve the Zero Amount First

Lines of code Vulnerability details Impact The PooledCreditLine.sol and LenderPool.sol contracts approve the strategy contract on the collateral or borrow assets before depositing funds. This allows the strategy contracts to transfer on behalf of these contracts, pulling assets out from the pool...

Incorrect value subtracted from sharesHeld for closed and liquidated pools in LenderPool._withdrawLiquidity

Lines of code Vulnerability details Impact sharesHeld is used to track the amount of yield shares held by LenderPool. However, in the withdrawLiquidity function, when both principal and interest are withdrawn, sharesHeld is only modified by the interest part. This results in incorrect bookkeeping...

updateSignValidity() May Break registerSelf() Due to Lack of Input Validation

Lines of code Vulnerability details Impact requireblock.timestamp Recommended Mitigation Steps requiresignValidity != 0, "signValidity Can't Be Zero" --- The text was updated successfully, but these errors were encountered: All reactions...

Potentially depositing at unfavorable rate since anyone can deposit the entire lenderPool to a known strategy at a pre-fixed time

Lines of code Vulnerability details Impact An attacker could keep track of the totalSupply of each LenderPool to see if it is more than the minBorrowAmount. If so, at startTime, which is pre-announced, the attacker could call start, which will trigger SAVINGSACCOUNT.deposit of the entire pool...

Result of transfer not checked

Lines of code Vulnerability details Impact A call to transfer is done without checking the results. For certain ERC20 tokens, if insufficient tokens are present, no revert occurs but a result of "false" is returned. So it's important and also a best practice to check this. Note that, in almost al...

DoS: Attacker May Front-Run createSplit() With A merkleRoot Causing Future Transactions With The Same merkleRoot to Revert

Lines of code Vulnerability details Impact A merkleRoot may only be used once in createSplit since it is used as salt to the deployment of a SplitProxy. The result is an attacker may front-run any createSplit transaction in the mem pool and create another createSplit transaction with a higher gas...

createProject can be frontrun

Lines of code Vulnerability details Impact This is dangerous in scam senario because the malicious user can frontrun and become the owner of the collection. As owner, one can withdraw paymentToken. note that collections.isForSale can be change by frontrunner Proof of Concept 1. Anyone can call...

DoS: Attacker May Front-Run CoreFactory.createProject() Or CoreFactory.addCollection() With A collection.id Causing Future Transactions With The Same collection.id to Revert

Lines of code Vulnerability details Impact A collection.id may only be used once in CoreFactory.createCollection since the the contract is deployed using the create2 opcode with a repeated salt and contract bytecode will fail to deploy a contract. Furthermore, the modifier onlyAvailableCollection...

Users at UNSTAKE_PERIOD can assist other users in unstaking tokens.

Lines of code Vulnerability details Impact Consider the following scenario: Day 0: User A stakes 200 tokens and calls the cooldown function. At this time, user A's cooldown is Day 0. Day 15: User B stakes 100 tokens, but then wants to unstake tokens. So user A said that he could assist user B in...

Ineffective Handling of FoT or Rebasing Tokens

Lines of code Vulnerability details Impact Certain ERC20 tokens may change user's balances over time positively or negatively or charge a fee when a transfer is called FoT tokens. The accounting of these tokens is not handled by RoyaltyVault.sol or Splitter.sol and may result in tokens being stuc...

Multiple missing approve(spender, 0) before approve(spender, amount)

Lines of code Vulnerability details Impact There are a few instances of missing calls to the IERC20 interface's approve function. The allowance needs to be set to 0 by calling for a given address and spender IERC20address.approvespender, 0. Not only do tokens like USDT require the allowance to be...

Incorrect borrowLimit used in calculating principal to withdraw for non-activated pools that are terminated

Lines of code Vulnerability details Impact LenderPool does not check the status of CreditLine when admin calls terminate. Thus if a careless admin discovered some malicious borrower and accidentaly terminated the pool before it went active, excessive tokens will be withdrawn, resulting in stolen...

[WP-M10] Lack of access control allow anyone to withdrawInterest() for any lender

Lines of code Vulnerability details function withdrawInterestuint256 id, address lender external nonReentrant withdrawInterestid, lender; function withdrawInterestuint256 id, address lender internal address strategy = pooledCLConstantsid.borrowAssetStrategy; address borrowAsset =...

_withdrawLiquidity() is Not Consistent With start()

Lines of code Vulnerability details Impact withdrawLiquidity details a number of scenarios under which a user may withdraw their liquidity. The first two scenarios outline cases where the credit line has either been cancelled by the borrower or insufficient assets have been lent out to the pool b...

[WP-H3] Proxy admin of the upgradeable proxy contracts can steal _borrowAsset and collateralAsset from the contracts and users' wallet

Lines of code Vulnerability details Both LenderPool and PooledCreditLine are upgradeable contract that holds users' allowances, and in certain periods, LendingPool will be holding users' funds. Use of Upgradeable Proxy Contract Structure allows the logic of the contract to be arbitrarily changed...

Denial of services in proxy context by setting immutable privileged addresses in constructor in upgradeable contracts

Lines of code Vulnerability details Impact Privileged immutable addresses in LenderPool such as POOLEDCREDITLINE, SAVINGSACCOUNT and VERIFICATION are set in the constructor in the logic contract. These values are run at the time of deployment and affect only the local storage of the logic contrac...

cooldown is set to 0 when the user sends all tokens to himself.

Lines of code Vulnerability details Impact In the beforeTokenTransfer function, cooldowns will be set to 0 when the user transfers all tokens to himself. Consider the following scenario Day 0: The user stakes 100 tokens and calls the cooldown function Day 10: the user wanted to unstake the tokens...

Pool Credit Line May Not Able to Start When _borrowAsset is Non ERC20 Compliant Tokens

Lines of code Vulnerability details Impact IERC20borrowAsset.transferto, fee; If the USDT token is supported as borrowAsset, the unsafe version of .transferto, fee may revert as there is no return value in the USDT token contract’s transfer implementation but the IERC20 interface expects a return...

Reentrancy

Lines of code Vulnerability details Impact Potential Reentrancy in staking/unstaking function Proof of Concept Reentrancy in HolyPaladinToken.stakeAndIncreaseLock contracts/HolyPaladinToken.sol346-365: External calls: - stakedAmount = stakemsg.sender,amount contracts/HolyPaladinToken.sol353...

PooledCreditLine: termination likely fails because _principleWithdrawable is treated as shares

Lines of code Vulnerability details Details & Impact principalWithdrawable is denominated in the borrowAsset, but subsequently treats it as the share amount to be withdrawn. // notBorrowed = borrowAsset amount that isn't borrowed // totalSupplyid = ERC1155 total supply of id // borrowedTokens =...

Add a timelock to DiamondCutFacet

Lines of code Vulnerability details Impact To give more trust to users: functions that set key/critical variables should be put behind a timelock. Proof of Concept Tools Used Remix Recommended Mitigation Steps Add a timelock to setter functions of key/critical variables. --- The text was updated...

use of transfer() instead of call() to send eth

Lines of code Vulnerability details Impact Use of transfer might render ETH impossible to withdraw becuase after istanbul hardfork , there is increases in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback...

ALMOST DEPRECATED TRANSFER() IS USED TO WITHDRAW ETHER

Lines of code Vulnerability details Impact transfer function can cause withdrawal to fail Proof of Concept function withdraw address assetAddress, address to, uint256 amount public LibDiamond.enforceIsContractOwner; address sendTo = to == address0 ? msg.sender : to; uint256 assetBalance; if...

Using the native payable.transfer to send ETH in WithdrawFacet

Lines of code Vulnerability details Impact The withdraw function in WithdrawFacet uses the native transfer keyword to send ETH, which is considered unsafe because of the fixed gas budget, and its functionality could be broken in some circumstances: 1. The receiver consumes more than 2300 amounts ...

Owner can whitelist addresses for swaps and steal approved assets from users

Lines of code Vulnerability details Impact There is a common vulnerability with aggregator/bridge contracts where passing in arbitrary calldata can do unwanted actions such as steal tokens that were approved to that contract. While there is a whitelist system set up, there is no stopping a...

Enforced Owner Can Extract Funds From The Contract

Lines of code Vulnerability details Impact During the code review, It has been observed that access control mechanisms are checked with the following line. LibDiamond.enforceIsContractOwner; The withdraw gaves abilitiy to contract owner extract all funds are sent to contract. This poses...

Should prevent users from sending more native tokens in the startBridgeTokensViaCBridge function

Lines of code Vulnerability details Impact When a user bridges a native token via the startBridgeTokensViaCBridge function of CBridgeFacet, the contract checks whether msg.value = cBridgeData.amount holds. In other words, if a user accidentally sends more native tokens than he has to, the contrac...

if msg.value > amount , then extra eth is not transfered back to user

Lines of code Vulnerability details Impact if msg.value amount , there is no mechanism to send extra eth back due to which user will lose extra Proof of Concept function startBridgeTokensViaCBridgeLiFiData memory lifiData, CBridgeData calldata cBridgeData public payable if cBridgeData.token !=...

Using payable.transfer functions in WithdrawFacet.sol and Libasset.sol is not usable for smart contract calls due to possible shortage of gas.

Lines of code Vulnerability details Impact Withdrawals and transferERC20 tokens are executed via transferERC20 and withdraw functions. Since these functions calls with a fixed amount of gas, it's not yet guaranteed to reach to the destination if the receiver is a smart contract. Proof of Concept...

Improper Token Balance Check on swap()

Lines of code Vulnerability details Improper Token Balance Check on swap Description The swap can be separated in 2 paths, swap native to ERC20, or swap ERC20 to native. The contract performs a fromAssetId balance check before calling swap, mean that the attacker could periodically check for ERC2...

[WP-H8] Admin of the upgradeable proxy contract (the diamond contract) can rug users

Lines of code Vulnerability details Use of Upgradeable Proxy Contract Structure The Diamond Structure allows the logic of the contract to be arbitrarily changed. This allows the proxy admin to perform malicious actions e.g., taking funds from users' wallets up to the allowance limit. This action...

User will lose value on swap-and-bridge / multi-swap

Lines of code Vulnerability details Impact In a swap-and-bridge or multi-swap, user have to supply exact amount of input token for each step. Any positive slippage will be captured by the contract and any negative slippage will cause the swap to revert. This is a sub-optimial behavior as it will...

Swap Functions Do Not Verify Final Token Matches The Swapped Token

Lines of code Vulnerability details Impact When calling Swapper.executeSwaps there are no checks to ensure the received token matches the final swapped token. If these are different it may result in user funds being locked in the contract. This issue is present in each of the following functions:...

Incorrect implementation of the batchRemoveDex function in DexManagerFacet

Lines of code Vulnerability details Impact The batchRemoveDex function does not work as expected. It should remove all the given DEX addresses from the dexWhitelist. However, it only removes the first successfully found DEX address and then stops removing the rest. The functionality is broken, an...

Any user can recover the funds left in the contract

Lines of code Vulnerability details Impact There is a WithdrawFacet such that only the owner/admin can recover the lost funds in the contract. However, any user can retrieve the funds by using the swapTokensGeneric function, which might be unexpected behavior. Proof of Concept 1. Suppose that 100...

GenericSwapFacet misuses _lifiData

Lines of code Vulnerability details Impact https://github.com/code-423n4/2022-03-lifinance/blob/main/docs/GenericSwapFacet.md stated that lifiData is strictly for analytics purposes. But lifiData is used to set receivingAsset. Proof of Concept In GenericSwapFacet.swapTokensGeneric,...