Juicebox V2 contest - codexploder's results

Lines of code

https://etherscan.io/address/0x729eE70bfdF65bEc7A530Fd49F644d07D0b2c087#L42 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5#L1

Vulnerability details

Impact

latestRoundData misses proper validation and is used actively in ethereum mainnet address at 0x729eE70bfdF65bEc7A530Fd49F644d07D0b2c087

Proof of Concept

1. Observe that currentPrice function is accepting the value from latestRoundData without performing any checks of stale price

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

    // Get a reference to the number of decimals the feed uses.
    uint256 _feedDecimals = feed.decimals();

    // Return the price, adjusted to the target decimals.
    return uint256(_price).adjustDecimals(_feedDecimals, _decimals);
  }

Recommended Mitigation Steps

Change the function to:

(uint80 round, int256 _price, , uint256 latestTimestamp, uint80 answeredInRound) = feed.latestRoundData();
require(_price> 0, "price <= 0");
require(answeredInRound >= round, "Stale price");
require(latestTimestamp != 0, "Round not complete");

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L81

Vulnerability details

Impact

There is no check to verify if the transfer was success or not. User calling _transferFrom will believe that transfer was success even when it has failed

Proof of Concept

1. Notice that _transferFrom function uses standard transfer and transferFrom function without checking there return value.

function _transferFrom(
    address _from,
    address payable _to,
    uint256 _amount
  ) internal override {
    _from == address(this)
      ? IERC20(token).transfer(_to, _amount)
      : IERC20(token).transferFrom(_from, _to, _amount);
  }

2. Even if result of transfer is false, contract has no way to know and would believe that transfer succeeded which is incorrect

Recommended Mitigation Steps

Always check return value of transfer/transferFrom function and revert if they return false

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L109

Vulnerability details

Impact

There is no way to change existing currency base price feed. In case of any bug identified in current feed address, this contract will need to be stuck in old buggy address

Proof of Concept

1. Observe the addFeedFor function

2. Assume owner added a new feed feedFor[_currency][_base] = _feed;

3. After some time a new security issue was discovered in _feed address

4. Owner wants to update the feed so that incorrect prices dont get returned due to security issue

5. However due to below require condition owner is unable to update _feed

if (feedFor[_currency][_base] != IJBPriceFeed(address(0))) revert PRICE_FEED_ALREADY_EXISTS();

Recommended Mitigation Steps

Implement a new function which allows users to modify existing currency base price feed

Multiple Tokens can have same symbol & Name

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBTokenStore.sol#L188

Function: issueFor

Issue: 2 tokens can have same symbol and name. This means one project can steal the token identity of another project

Recommendation: If a name/symbol pair is already in existence then token should not get created

Input Validation missing

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBProjects.sol#L168

Function: setMetadataOf

Issue: No check to verify if metadata content or domain is empty

Recommendation: Add a check to see that content is not missing

    if (bytes(_metadata.content).length > 0)
      metadataContentOf[projectId][_metadata.domain] = _metadata.content;

Incorrect bits mentioned in comment

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L317

Function: _getStructsFor

Recommendation: // projectId in bits 32-89. should be // projectId in bits 34-89.

Zero address check missing

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSplitsStore.sol#L253

Function: _set

Issue: Right now beneficiary of a split can be set to address zero

Recommendation: check _splits[_i].beneficiary is not address(0)

require(_splits[_i].beneficiary!=address(0), "Beneficiary cannot be 0");

Incorrect comment

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBController.sol#L421

Recommendation: Change to // Mint the project into the wallet of the owner.

Un-required iteration

Contract: https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBDirectory.sol#L276

Function: setTerminalsOf

Issue: _j < _terminals.length -1 since j is always running as i+1 which means last interaction of i loop will be _terminals.length -1 at which j should only run uptil _terminals.length -2 since j=i+1

Recommendation: Change loop as below:

for (uint256 _j = _i + 1; _j < _terminals.length-1; _j++)