Juicebox V2 contest - simon135's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/248fa2470ebd3f52c87c16a0cf2946e46f997397/contracts/JBChainlinkV3PriceFeed.sol#L44

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 price and latestRoundData can be stale with out knowing it.

Proof of Concept

  function currentPrice(uint256 _decimals) external view override returns (uint256) {
    // Get the latest round information. Only need the price is needed.
    (, int256 _price, , , ) = feed.latestRoundData();

This could lead to stale prices according to the Chainlink documentation: https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Tools Used

Vim

Recommended Mitigation Steps

At best use this

  require( price  > 0,"Chainlink answer reporting 0");

If want to make sure everything is correct get the roundID and timestamp to totally make sure there is no stale data.

   require(answeredInRound >= roundID, "Stale price");
        require(timestamp != 0,"Round not complete");
        require(price > 0,"Chainlink answer reporting 0");

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/c363abb67302314c2e061a01f76eb5e5dce2c935/contracts/JBTokenStore.sol#L188 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L562

Vulnerability details

impact

an attacker can make their own project but with same symbol and name and make it look execaully like a real project that its using juicebox and with the fake project the attacker can steal users funds

proof of concept

function issueTokenFor(

    uint256 _projectId,

    string calldata _name,

    string calldata _symbol

  )

    external

    virtual

    override

    requirePermission(projects.ownerOf(_projectId), _projectId, JBOperations.ISSUE)

    returns (IJBToken token)

  {

    // Issue the token in the store.

    return tokenStore.issueFor(_projectId, _name, _symbol);

  }

function issueFor(

    uint256 _projectId,

    string calldata _name,

    string calldata _symbol

  ) external override onlyController(_projectId) returns (IJBToken token) {

    // There must be a name.

    if (bytes(_name).length == 0) revert EMPTY_NAME();

  

    // There must be a symbol.

    if (bytes(_symbol).length == 0) revert EMPTY_SYMBOL();

  

    // The project shouldn't already have a token.

    if (tokenOf[_projectId] != IJBToken(address(0))) revert PROJECT_ALREADY_HAS_TOKEN();

  

    // Deploy the token contract.

    token = new JBToken(_name, _symbol);

  

    // Store the token contract.

    tokenOf[_projectId] = token;

  

    // Store the project for the token.

    projectOf[token] = _projectId;

ex:

1. an attacker makes a project then calls issueFor function
2. there is juicebox project aave that the attacker wants to phish ,the attacker supplies _name and _symbol as the same as aave
3. users see the fake one and the real one but they dont know which one is real. the only way users will know is if they can tell by the contracts .
4. bob is not so tech advanced and didnt check the contracts ,so bob picks the fake one and looses his funds

Tool Used

vim

Recommended Mitigation Steps

I would make sure on the front end side that _name, _symbol have to be orignal ( no same name or symbol can be used twice) or you can have the frontend make verfied projects that show up on the frontend to mitigate most of this risk.

JBChainlinkV3PriceFeed.sol

make constructor variable imutable because its not changed after deployment

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/248fa2470ebd3f52c87c16a0cf2946e46f997397/contracts/JBChainlinkV3PriceFeed.sol#L61

JbController.sol

make mappings into structs to save gas

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L64-L101

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled. i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2 But ++i returns the actual incremented value: uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; i++) {
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

make variables uninitlized to save gas

sstore: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/5ddb8136372ba06b5afe29156bff898335dd1fd1/contracts/JBProjects.sol#L40 mmstore:

abstract/JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:25:  using JBGlobalFundingCycleMetadataResolver for uint8;
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; i++) {
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Unchecking arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow

abstract/JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:25:  using JBGlobalFundingCycleMetadataResolver for uint8;
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; i++) {
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

make ,for loop array cached to save gas

abstract/JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
abstract/JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
JBDirectory.sol:25:  using JBGlobalFundingCycleMetadataResolver for uint8;
JBDirectory.sol:139:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
JBDirectory.sol:167:    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
JBDirectory.sol:275:      for (uint256 _i; _i < _terminals.length; _i++)
JBDirectory.sol:276:        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; i++) {
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

make address(0) into the long way

0x0000000000000 to save gas

JBTokenStore.sol:123:    if (_token != IJBToken(address(0))) totalSupply = totalSupply + _token.totalSupply(_projectId);
JBTokenStore.sol:148:    if (_token != IJBToken(address(0))) balance = balance + _token.balanceOf(_holder, _projectId);
JBTokenStore.sol:200:    if (tokenOf[_projectId] != IJBToken(address(0))) revert PROJECT_ALREADY_HAS_TOKEN();
JBTokenStore.sol:242:    if (_token == IJBToken(address(0)) && requireClaimFor[_projectId])
JBTokenStore.sol:249:    if (_token != IJBToken(address(0)) && _token.decimals() != 18)
JBTokenStore.sol:259:    if (_token != IJBToken(address(0))) projectOf[_token] = _projectId;
JBTokenStore.sol:262:    if (oldToken != IJBToken(address(0))) projectOf[oldToken] = 0;
JBTokenStore.sol:265:    if (_newOwner != address(0) && oldToken != IJBToken(address(0)))
JBTokenStore.sol:294:      _token != IJBToken(address(0));
JBTokenStore.sol:333:    uint256 _claimedBalance = _token == IJBToken(address(0))
JBTokenStore.sol:400:    if (_token == IJBToken(address(0))) revert TOKEN_NOT_FOUND();
JBTokenStore.sol:439:    if (_recipient == address(0)) revert RECIPIENT_ZERO_ADDRESS();
JBTokenStore.sol:477:    if (_token == IJBToken(address(0))) revert TOKEN_NOT_FOUND();
JBPrices.sol:69:    if (_feed != IJBPriceFeed(address(0))) return _feed.currentPrice(_decimals);
JBPrices.sol:75:    if (_feed != IJBPriceFeed(address(0)))
JBPrices.sol:115:    if (feedFor[_currency][_base] != IJBPriceFeed(address(0))) revert PRICE_FEED_ALREADY_EXISTS();
JBProjects.sol:71:    if (tokenUriResolver == IJBTokenUriResolver(address(0))) return '';
JBFundingCycleStore.sol:346:      _data.ballot != IJBFundingCycleBallot(address(0)) ||
JBETHERC20SplitsPayer.sol:536:              _split.beneficiary != address(0) ?
JBETHERC20SplitsPayer.sol:480:        if (_split.allocator != IJBSplitAllocator(address(0))) {
JBController.sol:930:          _split.allocator != IJBSplitAllocator(address(0))
JBController.sol:934:            : _split.beneficiary != address(0)
JBController.sol:943:        if (_split.allocator != IJBSplitAllocator(address(0)))
JBDirectory.sol:103:    @return An array of terminal addresses.
JBDirectory.sol:134:      _primaryTerminalOf[_projectId][_token] != IJBPaymentTerminal(address(0)) &&
JBDirectory.sol:145:    return IJBPaymentTerminal(address(0));
JBDirectory.sol:180:    @param _owner The address that will own the contract.
JBDirectory.sol:206:    - or, an allowedlisted address is setting a controller for a project that doesn't already have a controller.
JBDirectory.sol:219:        (isAllowedToSetFirstController[msg.sender] && controllerOf[_projectId] == address(0)))
JBDirectory.sol:230:      msg.sender != address(controllerOf[_projectId]) &&
JBDirectory.sol:231:      controllerOf[_projectId] != address(0) 

make onlyowner functions to payable to save gas

becuase there is no check msg.value ==0

abstract/JBPayoutRedemptionPaymentTerminal.sol:622:  function setFee(uint256 _fee) external virtual override onlyOwner {
abstract/JBPayoutRedemptionPaymentTerminal.sol:644:  function setFeeGauge(IJBFeeGauge _feeGauge) external virtual override onlyOwner {
abstract/JBPayoutRedemptionPaymentTerminal.sol:661:  function setFeelessAddress(address _address, bool _flag) external virtual override onlyOwner {
JBDirectory.sol:336:    onlyOwner
JBETHERC20ProjectPayer.sol:203:  ) external virtual override onlyOwner {
JBETHERC20SplitsPayer.sol:209:  ) external virtual override onlyOwner {
JBPrices.sol:113:  ) external override onlyOwner {
JBProjects.sol:179:  function setTokenUriResolver(IJBTokenUriResolver _newResolver) external override onlyOwner {
JBToken.sol:110:  ) external override onlyOwner {
JBToken.sol:131:  ) external override onlyOwner {
JBToken.sol:211:    onlyOwner

Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.

JBTokenStore.sol:101: mapping(uint256 => bool) public override requireClaimFor;

make 10**18 into 1e18 to save gas

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBSingleTokenPaymentTerminalStore.sol#L868

for all the solidity version up grade 0.8.10 and above for some optimzations

 JBChainlinkV3PriceFeed.sol  JBERC20PaymentTerminal.sol          JBETHERC20SplitsPayerDeployer.sol  JBFundingCycleStore.sol  JBProjects.sol                         JBSplitsStore.sol  libraries
enums       JBController.sol            JBETHERC20ProjectPayerDeployer.sol  JBETHERC20SplitsPayer.sol          JBOperatorStore.sol      JBReconfigurationBufferBallot.sol      JBToken.sol        structs
interfaces  JBDirectory.sol             JBETHERC20ProjectPayer.sol          JBETHPaymentTerminal.sol           JBPrices.sol             JBSingleTokenPaymentTerminalStore.sol  JBTokenStore.sol