Juicebox V2 contest - jonatascm's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L42-L51

Vulnerability details

Impact

ChainlinkAggregator fetches the price in JBChainlinkV3PriceFeed using the latestRoundData function. However, there are no checks on roundID nor timeStamp, resulting in stale prices. The contract JBPrices use currentPrice function to return the current price of a token and might return an incorrect value.

Recommended Mitigation Steps

function currentPrice(uint256 _decimals) external view override returns (uint256) {
  // Get the latest round information. Only need the price is needed.
- (, int256 _price, , , ) = feed.latestRoundData();
+	(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = feed.latestRoundData();
+ require(price > 0, "invalid price"); 
+ require(answeredInRound >= roundID, "invalid answeredInRound");
+ require(timeStamp != 0, "invalid timestamp");

  // 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);
}

Lines of code

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

Vulnerability details

Vulnerability details

Some tokens fail silently, not throwing an error. The functions safeTransfer and safeTransferFrom has checks for that returns and throw an error if not possible to transfer the tokens

Recommended Mitigation Steps

+ using SafeERC20 for IERC20;

...
_from == address(this)
-      ? IERC20(token).transfer(_to, _amount)
-      : IERC20(token).transferFrom(_from, _to, _amount);
+      ? IERC20(token).safeTransfer(_to, _amount)
+      : IERC20(token).safeTransferFrom(_from, _to, _amount);

Should check if owner is not null in launchProjectFor in JBController

Target codebase

JBController.sol#L410-L422

Vulnerability details

Is able to create a null owner to the project, should check if the _owner is set before create a new project.

Recommended Mitigation Steps

) external virtual override returns (uint256 projectId) {
+  if (_owner == address(0)) revert CUSTOM_ERROR();
    
  // Mint the project into the wallet of the message sender.
  projectId = projects.createFor(_owner, _projectMetadata);
	...
}

---

Should check constructor variables before assign

Target codebase

All contracts

Vulnerability details

Is possible to create the contracts without setting the corrects immutable variables.

Recommended Mitigation Steps

Should check for variables in constructor for all contracts, example:

In JBTokenStore.sol

constructor(
  IJBOperatorStore _operatorStore,
  IJBProjects _projects,
  IJBDirectory _directory
) JBOperatable(_operatorStore) JBControllerUtility(_directory) {
+ if(project == IJBProjects(address(0))) revert NO_PROJECT_SET;
  projects = _projects;
}

---

Don't need to use named return if not using it

Target codebase

JBPayoutRedemptionPaymentTerminal.sol#L444

Vulnerability details

Return named variables should be used or removed

Recommended Mitigation Steps

Remove the named return

- ) external virtual override returns (uint256 netLeftoverDistributionAmount) {
+ ) external virtual override returns (uint256) {

or add the named return instead of return, like this code:

- return _distributePayoutsOf(_projectId, _amount, _currency, _minReturnedTokens, _memo);
+ netLeftoverDistributionAmount = _distributePayoutsOf(_projectId, _amount, _currency, _minReturnedTokens, _memo);

Optimize loops in code

Target codebase

JBController.sol#L913

JBController.sol#L1014

JBOperatorStore.sol#L85

JBOperatorStore.sol#L135

JBOperatorStore.sol#L165

JBSingleTokenPaymentTerminalStore.sol#L862

JBSplitsStore.sol#L165

JBSplitsStore.sol#L204

JBSplitsStore.sol#L211

JBSplitsStore.sol#L229

JBSplitsStore.sol#L304

JBDirectory.sol#L275-L276

JBPayoutRedemptionPaymentTerminal.sol#L594

JBPayoutRedemptionPaymentTerminal.sol#L1008

Vulnerability details

Most of all loops in project aren't optimized

Recommended Mitigation Steps

Fix all loops following the steps:

1. Cache array length
2. change _i ++ for unchecked
3. not initialize _i = 0

- for (uint256 _i = 0; _i < array.length(); _i++) {...}
+ uint256 length = array.length();
+ for (uint256 _i; _i < length;){
+   ...
+   unchecked { ++_i; }
+ }

---

Cache storage variables in loop

Target codebase

JBSplitsStore.sol#L204-L223

JBSplitsStore.sol#L227-L274

JBPayoutRedemptionPaymentTerminal.sol#L594-L611

JBPayoutRedemptionPaymentTerminal.sol#L1396-L1428

Vulnerability details

Caching storage variables into memory variable can save gas when accessed more than 1 time in loop

Recommended Mitigation Steps

//JBSplitsStore.sol#L204-L223
for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
+ JBSplit memory currentSplit = _currentSplits[i];
	...
	for (uint256 _j = 0; _j < _splits.length; _j++) {
	  // Check for sameness.
+   JBSplit memory split = _splits[j]
	...
	}
}

//JBSplitsStore.sol#L227-274
for (uint256 _i = 0; _i < _splits.length; _i++) {
+	JBSplit memory split = _splits[i];
	...
}

//JBPayoutRedemptionPaymentTerminal.sol#L596-L612
for (uint256 _i = 0; _i < _heldFeeLength; ) {
+ JBFee memory heldFee = _heldFees[_i];
	...
}

JBPayoutRedemptionPaymentTerminal.sol#L1398-L1427
for (uint256 _i = 0; _i < _heldFeesLength; ) {
+ JBFee memory heldFee = _heldFees[_i];
	...
}

---

Variables don't need to be declared with default value

Target codebase

JBProjects.sol#L40

JBSplitsStore.sol#L227

JBSingleTokenPaymentTerminalStore.sol#L730

Vulnerability details

Variables declared with default value aren't gas optimized, they shouldn't be declared with default value

Recommended Mitigation Steps

Follow the changes to optimize the code:

- uint256 x = 0;
+ uint256 x;

- bool flag = false;
+ bool flag;

mapping(uint256 => uint256) map;
- map[x] = 0;
+ delete map[x];

---

Change feed to immutable in JBChainlinkV3PriceFeed

Target codebase

JBChainlinkV3PriceFeed.sol#L28

Vulnerability details

Variables declared as immutable saves gas

Recommended Mitigation Steps

- AggregatorV3Interface public feed;
+ AggregatorV3Interface public immutable feed;

---

Optimize if condition in JBTokenStore

Target codebase

JBTokenStore.sol#L353-L364

Vulnerability details

Simplifying the if/else conditions can save gas

Recommended Mitigation Steps

// The amount of tokens to burn.
uint256 _claimedTokensToBurn;
- if (_claimedBalance == 0)
-  _claimedTokensToBurn = 0;
- else if (_preferClaimedTokens)
-  _claimedTokensToBurn = _claimedBalance < _amount ? _claimedBalance : _amount;
- else _claimedTokensToBurn = _unclaimedBalance < _amount ? _amount - _unclaimedBalance : 0;

+ if (_claimedBalance != 0) {
+ 	if (_preferClaimedTokens)
+ 	  _claimedTokensToBurn = _claimedBalance < _amount ? _claimedBalance : _amount;
+ 	else _claimedTokensToBurn = _unclaimedBalance < _amount ? _amount - _unclaimedBalance : 0;
+ }