Juicebox V2 contest - 0x1f8b's results

Lines of code

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

Vulnerability details

Vulnerability

In JBChainlinkV3PriceFeed.sol#L43-L44, we are using latestRoundData, but there are no validations that the data is not stale.

The current code is:

    // Get the latest round information. Only need the price is needed.
    (, int256 _price, , , ) = feed.latestRoundData();

But is missing the checks to validate the data is stale

    // Get the latest round information. Only need the price is needed.
-   (, int256 _price, , , ) = feed.latestRoundData();
+   (uint80 round, int256 _price, , uint256 latestTimestamp, answeredInRound) = feed.latestRoundData();
    require(_price> 0, "Chainlink price <= 0");
    require(latestTimestamp != 0, "Incomplete round");
    require(answeredInRound >= round, "Stale price");

This could affect in all the logic, including funds.

Lines of code

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

Vulnerability details

Impact

JBERC20PaymentTerminal._beforeTransferTo is not compatible with Tether (USDT) implementation

Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT or CVX)'s approve() function will revert if the current approval is not zero, to protect against front-running changes of approvals.

function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

  // To change the approve amount you first have to reduce the addresses`
  //  allowance to zero by calling `approve(_spender, 0)` if it is not
  //  already 0 to mitigate the race condition described here:
  //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
  require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

  allowed[msg.sender][_spender] = _value;
  Approval(msg.sender, _spender, _value);
}

The code as currently implemented does not handle these sorts of tokens properly, which would prevent USDT, from being used by this project.

Reference:

• https://eips.ethereum.org/EIPS/eip-20

NOTE: To prevent attack vectors like the one described here and discussed here, clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender. THOUGH The contract itself shouldn’t enforce it, to allow backwards compatibility with contracts deployed before

Affected source code:

• JBERC20PaymentTerminal.sol#L99

Recommended Mitigation Steps

• Approve 0 first.

• Low
  • 1. Outdated packages
  • 2. Outdated compiler
  • 3. Lack of checks
  • 4. Lack of ACK during owner change
  • 5. Unsecure logic in JBSingleTokenPaymentTerminal
  • 6. Packages with vulnerabilities
  • 7. Use enum instead of constant
  • 8. Wrong desing
  • 9. Unsafe ERC20 calls
  • 10. Possible Denial of Service
• Non-Critical
  • 11. uint8 for decimals

Low

1. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts" : "^4.5.0-rc.0", => v4.7.0 "@chainlink/contracts" : "^0.1.6", => v0.4.1

2. Outdated compiler

The pragma version used is:

pragma solidity 0.8.6;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

3. Lack of checks

Check for address(0) during constructor, otherwise it could lead to bad initialization, bad implementations, or bad admin changes.

Affected source code:

address(0):

• JBDirectory.sol#L236
• JBDirectory.sol#L188-L191
• JBControllerUtility.sol#L53
• JBOperatable.sol#L78
• JBPayoutRedemptionPaymentTerminal.sol#L290-L296
• JBPrices.sol#L89
• JBChainlinkV3PriceFeed.sol#L60
• JBTokenStore.sol#L162
• JBSplitsStore.sol#L123
• JBReconfigurationBufferBallot.sol#L119
• JBSingleTokenPaymentTerminalStore.sol#L281-L283
• JBController.sol#L377-L381

4. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• JBPayoutRedemptionPaymentTerminal.sol#L43
• JBPrices.sol#L20
• JBProjects.sol#L27
• JBToken.sol#L21
• JBTokenStore.sol#L239
• JBDirectory.sol#L23

5. Unsecure logic in JBSingleTokenPaymentTerminal

The decimalsForToken and currencyForToken methods return the value even though the token is not accepted, it should be:

function decimalsForToken(address _token) external view override returns (uint256) {
+ if (_token != token) revert TOKEN_NOT_ACCEPTED();  // Check `acceptsToken`
return decimals;
}
function currencyForToken(address _token) external view override returns (uint256) {
+ if (_token != token) revert TOKEN_NOT_ACCEPTED();  // Check `acceptsToken`
return currency;
}

6. Packages with vulnerabilities

The project contains packages that urgently need to be updated because they contain important vulnerabilities.

npm audit:

69 vulnerabilities (21 moderate, **45 high**, 3 critical)
To address issues that do not require attention, run:
  npm audit fix

7. Use enum instead of constant

enum checks the accepted range of values, in constants it does not. So the user or developer may set a value out of range.

Affected source code:

• JBCurrencies.sol#L5-L6
• JBOperations.sol#L5-L22
• JBSplitsGroups.sol#L5-L6

8. Wrong desing

The user may think that they are approving for a particular project _projectId but they are actually approving for the account, so it is better to remove the argument. All ERC20 methods are affected by this unusable argument.

Affected source code:

• JBToken.sol#L146

9. Unsafe ERC20 calls

The following code doesn't check the result of the ERC20 calls. ERC20 standard specify that the token can return false if these calls fails, so it's mandatory to check the result of these ERC20 methods.

Reference:

• EIP-20

  NOTES: The following specifications use syntax from Solidity 0.4.17 (or above). Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

Affected source code for transfer and transferFrom:

• JBERC20PaymentTerminal.sol#L87-L88

Affected source code for approve:

• JBERC20PaymentTerminal.sol#L99

10. Possible Denial of Service

With a very low _baseFundingCycle.duration, such as 1, the loop may overgas with some values in _mustStartAtOrAfter and _baseFundingCycle in JBFundingCycleStore.sol#L686

Similarly in JBFundingCycleStore.sol#L755 certain configurations could produce a denial of service due to integer underflow..

---

Non-Critical

11. uint8 for decimals

Since the ERC20 sets decimals as a byte, the interface and the contract should use it with that type.

Affected source code:

• IJBSingleTokenPaymentTerminal.sol#L11
• JBSingleTokenPaymentTerminal.sol#L34

• Gas
  • 1. Reorder structure layout
  • 2. ++i costs less gas compared to i++ or i += 1
  • 3. There's no need to set default values for variables
  • 4. Optimize logic
  • 5. Gas saving using immutable
  • 6. Deadcode
  • 7. Delete optimization
  • 8. Reduce math operations

Gas

1. Reorder structure layout

The following structs could be optimized moving the position of certains values in order to save slot storages:

• JBSplit.sol#L15-L23

struct JBSplit {
  bool preferClaimed;
  bool preferAddToBalance;
+ address payable beneficiary; // < - Move close to bool values
  uint256 percent;
  uint256 projectId;
- address payable beneficiary;
  uint256 lockedUntil;
  IJBSplitAllocator allocator;
}

2. ++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 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• JBOperatorStore.sol#L85
• JBOperatorStore.sol#L135
• JBOperatorStore.sol#L165
• JBSplitsStore.sol#L204
• JBSplitsStore.sol#L211
• JBSplitsStore.sol#L229
• JBSplitsStore.sol#L304
• JBDirectory.sol#L139
• JBDirectory.sol#L167
• JBDirectory.sol#L275-L276
• JBFundingCycleStore.sol#L724
• JBSingleTokenPaymentTerminalStore.sol#L862
• JBController.sol#L913
• JBController.sol#L1014

3. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Affected source code:

• JBSplitsStore.sol#L165
• JBSplitsStore.sol#L204
• JBSplitsStore.sol#L211
• JBSplitsStore.sol#L229
• JBSplitsStore.sol#L304
• JBOperatorStore.sol#L85
• JBOperatorStore.sol#L135
• JBOperatorStore.sol#L165
• JBFundingCycleStore.sol#L724
• JBSingleTokenPaymentTerminalStore.sol#L862
• JBController.sol#L913

4. Optimize logic

Change JBOperatorStore.hasPermissions as follows to avoid redundant storage access:

function hasPermissions(address _operator, address _account, uint256 _domain, uint256[] calldata _permissionIndexes ) 
  external view override returns (bool) {
+   uint256 permissions = permissionsOf[_operator][_account][_domain];
    for (uint256 _i; _i < _permissionIndexes.length; ++_i) {
      uint256 _permissionIndex = _permissionIndexes[_i];

      if (_permissionIndex > 255) revert PERMISSION_INDEX_OUT_OF_BOUNDS();
-     if (((permissionsOf[_operator][_account][_domain] >> _permissionIndex) & 1) == 0)
+     if (((permissions >> _permissionIndex) & 1) == 0) return false;
    }
    return true;
}

Add break inside JBSplitsStore.sol#L220 loop condition:

if (
        _splits[_j].percent == _currentSplits[_i].percent &&
        _splits[_j].beneficiary == _currentSplits[_i].beneficiary &&
        _splits[_j].allocator == _currentSplits[_i].allocator &&
        _splits[_j].projectId == _currentSplits[_i].projectId &&
        // Allow lock extention.
        _splits[_j].lockedUntil >= _currentSplits[_i].lockedUntil
-       ) _includesLocked = true;
+       ) { _includesLocked = true; break; }

• JBSplitsStore.sol#L220

5. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• JBChainlinkV3PriceFeed.sol#L28

6. Deadcode

The logic to check that the address its a contract can be eliminated since a call to supportsInterface is then made, which would give the same error code.

-      address _ballot = address(_data.ballot);
-      uint32 _size;
-      assembly {
-        _size := extcodesize(_ballot) // No contract at the address ?
-      }
-      if (_size == 0) revert INVALID_BALLOT();
       try _data.ballot.supportsInterface(type(IJBFundingCycleBallot).interfaceId) returns (bool _supports) {
         if(!_supports) revert INVALID_BALLOT(); // Contract exists at the address but with the wrong interface
       } catch {
         revert INVALID_BALLOT(); // No ERC165 support
       }

• JBFundingCycleStore.sol#L317-L322

7. Delete optimization

Use delete instead of set to default value (false or 0).

Here you can see an example of this optimization where 5 gas is saved:

contract A  {
    mapping(uint256 => uint256) public balanceOf;
    constructor () { balanceOf[1] = 2; }
    function test1() public  
    {
        balanceOf[1] = 0;     // 21478 gas 
    }
}

contract B  {
    mapping(uint256 => uint256) public balanceOf;
    constructor () { balanceOf[1] = 2; }
    function test1() public  
    {
        delete balanceOf[1];  // 21473 Gas
    }
}

Affected source code:

• JBSingleTokenPaymentTerminalStore.sol#L730

8. Reduce math operations

Use scientific notation (e.g. 10e17) rather than exponentiation (e.g. 10**18)

10**18 => 10e17:

• JBSingleTokenPaymentTerminalStore.sol#L868