Juicebox V2 contest - 0x29A's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBChainlinkV3PriceFeed.sol#L34-L51

Vulnerability details

Impact

The _price provided by chainlink AggregatorV3 could be a negative, if that happend the cast of the _price goes high, in example, cast -1 to uint256 was 2**256 - 1

Proof of Concept

return uint256(_price).adjustDecimals(_feedDecimals, _decimals);

Tools Used

Manual Review

Recommended Mitigation Steps

Use a SafeCast library of openzeppelin toUint256(int256 value) or check the _price with a require before cast it: require(_price >= 0, "_price must be positive");

Lines of code

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

Vulnerability details

Impact

In the contract JBERC20PaymentTerminal, the return values of ERC20 transfer and transferFrom are not checked to be true, which could be false if the transferred tokens are not ERC20-compliant. In that case, the transfer fails without being noticed by the calling contract.

Out of scope but also found in JBETHERC20SplitsPayer.sol L256, L301, L348, L384, L534 and JBETHERC20ProjectPayer.sol L271, L301, L315, L384, L534

Proof of Concept

With the current implementation of _transferFrom, it could be that trasnferFrom return false and not revert when the sender does not have enough token to transfer (both for transfer and transferFrom).

Tools Used

Manual Review

Recommended Mitigation Steps

Use the SafeERC20 library implementation from OpenZeppelin and call safeTransfer or safeTransferFrom when transferring ERC20 tokens.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L415-L448 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L788-L900 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L981-L1174 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHPaymentTerminal.sol#L63-L79 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L73-L89

Vulnerability details

Impact

In the contract JBPayoutRedemptionPaymentTerminal, the function distributePayoutsOf calls the internal function _distributePayoutsOf and this internal function perfoms a loop where is using the function _distributeToPayoutSplitsOf In these functions there are a _transferFrom what:

• JBETHPaymentTerminal using a Address.sendValue(_to, _amount)
• JBERC20PaymentTerminal using a IERC20(token).transfer(_to, _amount) with a ERC777 as token

Both give back the control to the msg.sender (_to variable) creating a reentrancy attack vector

Also could end with a lot of bad calculation because is using uncheckeds statements and function _distributePayoutsOf its no respecting the checks, effects, interactions pattern

Proof of Concept

Craft a contract to call function distributePayoutsOf, on receive ether reentrant to function distributePayoutsOf or use a ERC777 callback.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a reentrancyGuard as you do on JBSingleTokenPaymentTerminalStore.sol; You have already import the ReentrancyGuard on JBPayoutRedemptionPaymentTerminal.sol#L5 but you are not using it.

My recommendation is to add nonReentrant modifier on function distributePayoutsOf

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L816 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L668 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L681 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L743 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L785 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/733810a0339a5c0cb608345e6fc66a6edeac13cc/contracts/JBController.sol#L859

Vulnerability details

Impact

The JBController contract performs many unsafe casts uint256 to int256 and int256 to uint256 In example:

• the cast -1(int256) to uint256 was 2**256 - 1
• the cast 2**255(uint256) to int256 was - 2**255

Proof of Concept

int256 to uint256:

• L816: if (uint256(_processedTokenTrackerOf[_projectId]) != tokenStore.totalSupplyOf(_projectId))

uint256 to int256:

• L668: int256(_tokenCount);
• L681: int256(beneficiaryTokenCount);
• L743: int256(_tokenCount);
• L785: _processedTokenTrackerOf[_projectId] = int256(tokenStore.totalSupplyOf(_projectId));
• L859: _processedTokenTrackerOf[_projectId] = int256(_totalTokens + tokenCount);

Note: in the L1076 and L1077 there are two more casts but in the L1075 check the cast

Tools Used

Review

Recommended Mitigation Steps

Use a SafeCast library of openzeppelin toUint256(int256 value) and toInt256(uint256 value) or check the number before cast it

[G-01] ++i costs less gas compared to i++ or i += 1 and use unchecked for boost save gas

++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.

Also using unchecked arithmetic will cost less gass because it wouldnt run a check for under/overflow and revert if it is detected.

Take in mind you shoul only use unchecked when its safe to increment without overflow, like in loops iterator.

Recommendation:

Replace this lines

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++)
JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; 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: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++) {
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++) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {

and use this pattern:

for (uint256 _i; _i < length;) {
    // code here
    unchecked { ++_i; }
}

[G-02] Use unchecked arithmetic

The default “checked” behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

Example

JBProjects.sol:137:    projectId = ++count;

This could be

JBProjects.sol:137:    unchecked { projectId = ++count; }

[G-03] Cache variables

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high.

diff --git a/contracts/JBDirectory.sol b/contracts/JBDirectory.sol
index e6d5a93..72a3b8c 100644
--- a/contracts/JBDirectory.sol
+++ b/contracts/JBDirectory.sol
@@ -129,15 +129,17 @@ contract JBDirectory is IJBDirectory, JBOperatable, Ownable {
     override
     returns (IJBPaymentTerminal)
   {
+    IJBPaymentTerminal _terminalToken =  _primaryTerminalOf[_projectId][_token];
     // If a primary terminal for the token was specifically set and its one of the project's terminals, return it.
     if (
-      _primaryTerminalOf[_projectId][_token] != IJBPaymentTerminal(address(0)) &&
-      isTerminalOf(_projectId, _primaryTerminalOf[_projectId][_token])
-    ) return _primaryTerminalOf[_projectId][_token];
+      _terminalToken != IJBPaymentTerminal(address(0)) &&
+      isTerminalOf(_projectId, _terminalToken)
+    ) return _terminalToken;
 
     // Return the first terminal which accepts the specified token.
-    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {
-      IJBPaymentTerminal _terminal = _terminalsOf[_projectId][_i];
+    IJBPaymentTerminal[] memory _terminals = _terminalsOf[_projectId];
+    for (uint256 _i; _i < _terminals.length; _i++) {
+      IJBPaymentTerminal _terminal = _terminals[_i];
       if (_terminal.acceptsToken(_token, _projectId)) return _terminal;
     }
 
@@ -164,8 +166,9 @@ contract JBDirectory is IJBDirectory, JBOperatable, Ownable {
     override
     returns (bool)
   {
-    for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++)
-      if (_terminalsOf[_projectId][_i] == _terminal) return true;
+    IJBPaymentTerminal[] memory _terminals = _terminalsOf[_projectId];
+    for (uint256 _i; _i < _terminals.length; _i++)
+      if (_terminals[_i] == _terminal) return true;
     return false;
   }
 
@@ -270,10 +273,11 @@ contract JBDirectory is IJBDirectory, JBOperatable, Ownable {
     // Delete the stored terminals for the project.
     _terminalsOf[_projectId] = _terminals;
 
+    uint256 _length = _terminals.length;
     // Make sure duplicates were not added.
-    if (_terminals.length > 1)
-      for (uint256 _i; _i < _terminals.length; _i++)
-        for (uint256 _j = _i + 1; _j < _terminals.length; _j++)
+    if (_length > 1)
+      for (uint256 _i; _i < _length; _i++)
+        for (uint256 _j = _i + 1; _j < _length; _j++)
           if (_terminals[_i] == _terminals[_j]) revert DUPLICATE_TERMINALS();
 
     emit SetTerminals(_projectId, _terminals, msg.sender);