Migration total supply reduction can be used to remove minority shareholders

Lines of code Vulnerability details As new total supply can be arbitrary, setting it significantly lower than current say to 100 when it was 1e9 before can be used to remove current minority shareholders, whose shares will end up being zero on a precision loss due to low new total supply value...

Not check fractionPrice > 0 may allow users to buy or sell fractions with zero ETH

Lines of code Vulnerability details Impact In Buyout.start function, fractionPrice is calculated from msg.value and depositAmount, both can be changed by sender. And there is no check if fractionPrice is bigger than 0. So in case fractionPrice = 0, users may sell and buy fractions with zero ETH...

Any token approved for the base vault can be stolen

Lines of code Vulnerability details Impact All tokens approved for the BaseVault contract can be stolen by any attacker. The batchDeposit functions lack proper access controls and allow the token source to be an arbitrary address as opposed to restricting it to msg.sender. Since token approvals...

percsion error that causes buyoutprice to be very low causing the fractionPrice very low or very high

Lines of code Vulnerability details percsion error that causes buyoutprice to be very low causing the fractionPrice very low detials how does buyout become realy low? because of this percison error in the code if you supply msg.value=1 depsitAmount=1 totalSupply=5 1. also there is a possiblity th...

Small scalar may make buyoutPrice calculation incorrectly

Lines of code Vulnerability details Impact In Buyout.start function, buyoutPrice is calculated using a scalar = 100. This small scalar and division rouding will reduce the precision of the formula significantly. Proof of Concept 1. Alice call start with msg.value = 1e18 and depositAmount = 4567...

unpaused(p) modifier missing in authRedeem function

Lines of code Vulnerability details Impact Due to missing modifier, User will be able to redeem zcTokens and withdraw underlying even in paused Market. This happens due to missing unpausedp modifier Proof of Concept 1. Lets see function definition for authRedeem function function authRedeemuint8 ...

Migration Module: Disable to join, leave, commit by starting a buyout

Lines of code Vulnerability details Impact MED - the function of the protocol could be impacted. Anyone can call Buyout::start to disable to join, leave, commit functions for migration proposal Proof of Concept The Buyout module is unaware of migration module. So, even when some migration is...

The FERC1155.sol don't respect the EIP2981

Lines of code Vulnerability details Impact The EIP-2981: NFT Royalty Standard implementation is incomplete, missing the implementation of function supportsInterfacebytes4 interfaceID external view returns bool; from the EIP-165: Standard Interface Detection Proof of Concept A marketplace...

Uninitialized implementation for Vault can be destroyed

Lines of code Vulnerability details Impact Every Vault is a proxy of the same implementation contract. This implementation is deployed from VaultFactory but never initialized. /// @notice Initializes implementation contract constructor implementation = addressnew Vault; Someone can call init in t...

User can increase migration's proposal total tokens artificially

Lines of code Vulnerability details Impact There are two functions that users can use to withdraw deposited ether/tokens from a joined proposal: leave and withdrawContribution. leave correctly decreases the total contribution from a proposal alongside the user's one: // Updates fraction balances ...

Native ETH transfer should use call() instead of transfer()

Lines of code Vulnerability details Impact It might be impossible for some addresses to receive ETH via transfer because receiver address might have methods that exceed 2300 gas, ultimately leading to frozen funds. Proof of Concept Native transfer function has a limit of 2300 gas, which might not...

Migration Module: Re-enter commit using custom token

Lines of code Vulnerability details Impact HIGH - Assets can be compromised directly. One can drain eth out from migration module to buyout module using custom made FERC1155 token. Proof of Concept proof of concept: testCommitReenterpoc custom made FERC1155 for the attack The proof of concept sho...

Migration Module: Usage of withdrawContribution instead of leave before buyout

Lines of code Vulnerability details Impact HIGH - Assets can be compromised directly. One can use eth from the module for buyout process. Basically sending somebody else's eth from migration module to buyout module Proof of Concept proof of concept The proof of concepts shows a scenario where one...

Migration Module: After successful migration, ERC20 assets can be thrown away by anyone

Lines of code Vulnerability details Impact HIGH - Assets can be lost directly After proposal and proposed buyout is successful, anyone can transfer ERC20 asset in the vault to the zero address and the asset will be lost. Proof of Concept proof of concept: testAnyoneCanThrowERC20poc The proof of...

Migration Module: The assets can be taken by a failed proposal

Lines of code Vulnerability details Impact HIGH - Assets can be stolen directly When there are multiple proposals for a vault, a failed proposal can withdraw the assets. Proof of Concept proof of concept: testMultiProposalpoc The proof of concept shows a scenario alice is taking assets. 1. setup:...

Migration's withdrawContribution ignores the exchange between fractional tokens and ETH happened during Buyout attempt

Lines of code Vulnerability details withdrawContribution aims to return the funds to Migration participants. However, it uses initial userProposalFractionsproposalIdmsg.sender and userProposalEthproposalIdmsg.sender records for withdrawal accounting. Real funds structure is different after Buyout...

Swivel withdraw wrapper function redeems an amount on underlying tokens

Lines of code Vulnerability details Impact Swivel contract uses the redeemUnderlying function from the Compound CERC20 tokens. This function takes as an input the amount of underlying tokens to be redeemed, which is inconsistent compared to other functions. This issue will lead to user fund losse...

TEST

Lines of code L1 Vulnerability details Impact Detailed description of the impact of this finding. Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Tools Used Recommended Mitigation Steps --- Th...

Upgraded Q -> M from 270 [1657580410834]

Judge has assessed an item in Issue 270 as Medium risk. The relevant finding follows: 6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user With PROTOCOLFEEBPS 10000 more than 100%, the exchange can steal user WETH who might approve max WETH...

Upgraded Q -> M from 270 [1657580270573]

Judge has assessed an item in Issue 270 as Medium risk. The relevant finding follows: 6.L- Admin config ProtocolFee and gasFee missing max amount check which can be used to take fund from user With PROTOCOLFEEBPS 10000 more than 100%, the exchange can steal user WETH who might approve max WETH...

Missing zero address check for bribesProcessor

Upgraded from 45: Missing zero address check for bribesProcessor MyStrategy.sol:100 ///@dev Change the contract that handles bribes function setBribesProcessorIBribesProcessor newBribesProcessor external onlyGovernance; bribesProcessor = newBribesProcessor; The bribeProcessor is not set in the...

Improper validation Chainlink's latestRoundData() for stale or incorrect result

Lines of code Vulnerability details Impact The result from price feed needs further validation for stale and incorrect results. Proof of Concept function currentPriceuint256 decimals external view override returns uint256 // Get the latest round information. Only need the price is needed. , int25...

Failed transfer during migrate could lockup the fund in old terminal

Lines of code Vulnerability details Without require for boolean checking, if to.addToBalanceOfvalue: payableValueprojectId, balance, token, '', bytes''; fail when transfer, migrate will not revert. This could cause the fund to lockup in the old terminal since store.recordMigrationprojectId; will...

Stale prices can be used by the system

Lines of code Vulnerability details updatedAt field of Chainlink's latestRoundData isn't checked, so even substantially outdated price will be used by the system. Proof of Concept Price is the only field that is read from Chainlink: function currentPriceuint256 decimals external view override...

Unchecked return value of transfer() and transferFrom()

Lines of code Vulnerability details Impact There are multiple external calls to IERC20.transfer and IERC20.transfer in contracts that suppose to transfer out tokens from/to the system. However, there are tokens like USDT that don't return any return value in their implementation of the transfer...

Users with the JBOperations.RECONFIGURE role can change the project controller if the project hasn't launched yet

Lines of code Vulnerability details Impact Only project owners, or users with the JBOperations.SETCONTROLLER role are supposed to be able to change the controller JBOperations.MIGRATION too if while doing a migration. The JBOperations.RECONFIGURE role is meant to be a less powerful role so this...

Weak PRNG (Pseudorandom number generator)

Lines of code Vulnerability details Impact Weak PRNG due to a modulo on block.timestamp, now or blockhash. These can be influenced by miners to some extent so they should be avoided. Proof of Concept Tools Used Slither Recommended Mitigation Steps Do not use block.timestamp, now or blockhash as a...

In JBERC20PaymentTerminal.sol#_transferFrom(...) ignoring return value when IERC20(token).transferFrom(...)

Lines of code Vulnerability details In JBERC20PaymentTerminal.soltransferFrom... ignoring return value when IERC20token.transferFrom... Some of ERC20 token implementations return false upon unsuccess transfer like USDT token, this can lead to some unpredictable balances to rise up without actual...

The reserves accounting breaks when total balances surpas type(int256).max

Lines of code Vulnerability details Reserved tokens do not get minted automatically when a new payment is received. Instead, they must be explicitly distributed during the funding cycle which contains the reserved rate and splits that should be applied. If a funding cycle's reserved rate or split...

Arbitrary msg.sender can execute recordPaymentFrom, recordRedemptionFor, recordDistributionFor, recordUsedAllowanceOf, recordAddedBalanceFor. Malicious terminal may be passed to inject logic to data store.

Lines of code Vulnerability details Impact Arbitrary msg.sender can execute recordPaymentFrom, recordRedemptionFor, recordDistributionFor, recordUsedAllowanceOf, recordAddedBalanceFor. Malicious terminal may be passed to inject logic to data store. If data store is using custom logic that doesn't...

JBProjects.sol#createFor(...) function is open to reentrancy attacks

Lines of code Vulnerability details JBProjects.solcreateFor... which used in JPController.sollaunchProjectFor... can be used for reentrancy. PoC contracts/JBProjects.sol: 131: function createForaddress owner, JBProjectMetadata calldatametadata 132 external 133 override 134 returns uint256 project...

Unchecked transfer

Lines of code Vulnerability details Description The return value of an external transfer/transferFrom call is not checked Impact There are some tokens that do not revert on failure but return false instead, if such token is used, the return value won'...

oldToken.transferOwnership() to contract may be irreversible

Lines of code Vulnerability details oldToken.transferOwnership to contract may be irreversible Proof of Concept JBTokenStore.sol 265-266: if newOwner != address0 && oldToken != IJBTokenaddress0 oldToken.transferOwnershipprojectId, newOwner; OpenZeppelin library Ownerable.sol: function...

Issue with condition check handling

Lines of code Vulnerability details Impact It might cause data corruption, as most of the data handling is done in low level. Proof of Concept // Add up all the percents to make sure they cumulative are under 100%. uint256 percentTotal = 0; for uint256 i = 0; i typeuint56.max revert...

State Variable Shadowing

Lines of code Vulnerability details Impact Check: shadowing-state Severity: High Confidence: High It is possible to use the same variable twice in Solidity, but it can lead to unintended side effects.The TestAllowance.sol contract inherits from TesTBaseWorkflow.sol. In the TestAllowance.sol...

Use of IERC20.transfer() instead of SafeERC20.safeTransfer()

Lines of code Vulnerability details Impact There are multiple external calls to IERC20.transfer and IERC20.transfer in contracts that suppose to transfer out tokens from/to the system. However, there are tokens like USDT that don't return any return value in their implementation of the transfer...

Insufficient oracle validation

Lines of code Vulnerability details Impact Oracle my return stale price. Proof of Concept Round completeness and the quoted timestamp are not checked to ensure that the reported price is not stale. roundId, startedAt, updatedAt, and answeredInRound are omitted from the return result of...

Missing inheritance for JBToken

Lines of code Vulnerability details Impact The JBToken contracts inherits IJBToken, however IJBToken does not inherit IERC20. This would cause the JBToken contract to not be ERC20 compliant and making many of the called ERC20 functions revert. Tools Used Manual review Recommended Mitigation Steps...

Price Feed is not checked for freshness and may report old / incorrect value

Lines of code Vulnerability details Price Feed is not checked for freshness In times of network conjestion, the priceFeed may take longer than expected to update, and the price may take longer than usual to update, in order to ensure the latest price is fresh within update window, you should veri...

DOS(Denial Of Service).External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract.

Lines of code Vulnerability details Impact --Check: calls-loop --Severity: Medium --Confidence: Medium External calls can fail accidentally or deliberately, which can cause a DoS condition in the contract. There are two instances where this can occur. Proof of Concept -- --ConsenSys Smart...

configureFor's check for interface compliance can backfire

Lines of code Vulnerability details Impact Controlller supplied data.ballot is checked by configureFor to be compliant with IJBFundingCycleBallot interface by wrapping the call. Proof of Concept configureFor checks for interface support with a nested function: try...

Migration to a new terminal allows project to use funding again in same funding cycle

Lines of code Vulnerability details Impact Distribution and overflow allowance can be reused after migration Proof of Concept When migrating to a new terminal only balances are transferred but not usedDistributionLimitOf or usedOverflowAllowanceOf. This means that both of these values will be 0,...

Project Owner can mint all tokens to their own address

Lines of code Vulnerability details Impact A project owner may mint all tokens for the project in JBController.mintTokensOf , with all tokens minted by the project owner to their own address, the payer during any pay call would be unable to to mint any project token as the mint function would...

Attacker can prevent other projects from using a custom token

Lines of code Vulnerability details Impact A malicious project owner or an attacker can front-run the JBTokenStore.changeFor function and "steal" the token for their own project. This token can then not be used for any other project as long as it's assigned to a project due to projectOftoken != 0...

Chainlink's latestRoundData (price) might return stale or incorrect result

Lines of code Vulnerability details JBChainlinkV3PriceFeed.sol we are using latestRoundData, but there is no check if the return value indicates stale data..Even though its only getting the price variable, the whole latestRoundData function gets returned and we cant just ignore it because the pri...

Oracle data feeds are insufficiently validated

Lines of code Vulnerability details Impact If the oracle price feeds are insufficiently validated, there will be pricing errors leading to the miss-pricing of assets Proof of Concept The JBSingleTokenPaymentTerminalStore and abstract JBPayoutRedemptionPaymentTerminal both rely on their respective...

No check for stale chainlink oracle data in currentPrice function

Lines of code Vulnerability details Impact There is no check if the value of price returned by chainlink latestRoundData is latest or stale. If stale price is returned, it may result in wrong calculation used further, and in JBPrices.sol Proof of Concept Contract : JBChainlinkV3PriceFeed.sol Line...

CHAINLINK’S LATESTROUNDDATA MIGHT RETURN STALE OR INCORRECT RESULTS

Lines of code Vulnerability details Impact This could lead to stale prices according to the Chainlink documentation: Proof of Concept On JBChainlinkV3PriceFeed.sol, we are using latestRoundData, but there is no check if the return value indicates stale data. function currentPriceuint256 decimals...

JBProjects.createFor should be able to set the project controller

Lines of code Vulnerability details Impact JBProjects.createFor doesn’t set the project controller. Project owners need to call JBDirectory.setControllerOf to set the project controller. However, an allowedlisted address can set the controller before the actual project owner. We can assume that...

Missing checks on return data from the chainlink

Lines of code Vulnerability details Impact MED - the function of the protocol could be impacted 1. Use stale price information resulting to wrong project's balance 2. In the case of zero price, functions using price information will revert. Proof of Concept // JBPrices::priceFor at line 69 calls...