Juicebox V2 contest - Ch_301's results

The decentralized fundraising and treasury protocol.

General Information

Platform: Code4rena

Start Date: 01/07/2022

Pot Size: $75,000 USDC

Total HM: 17

Participants: 105

Period: 7 days

Judge: Jack the Pug

Total Solo HM: 5

Id: 143

League: ETH

Juicebox

Findings Distribution

Researcher Performance

Rank: 45/105

Findings: 3

Award: $131.86

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Juicebox

Findings Information

🌟 Selected for report: horsefacts

Also found by: 0x1f8b, 0x29A, 0x52, 0xf15ers, AlleyCat, Ch_301, Chom, Franfran, IllIllI, Kaiziron, Limbooo, Meera, Ruhum, Sm4rty, apostle0x01, berndartmueller, cccz, cloudjunky, codexploder, cryptphi, delfin454000, durianSausage, fatherOfBlocks, hake, hansfriese, hyh, jonatascm, m_Rassska, oyc_109, peritoflores, rajatbeladiya, rbserver, svskaushik, zzzitron

Awards

3.4075 USDC - $3.41

Labels

bug

duplicate

2 (Med Risk)

valid

External Links

Link to GitHub issue

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBERC20PaymentTerminal.sol#L87-L88 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20ProjectPayer.sol#L271 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20ProjectPayer.sol#L315 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L256 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L301 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L348 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L384 https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L534

Vulnerability details

Details & Impact

The transferFrom() and transfer() method is used instead of safeTransferFrom() and safeTransfer()I however argue that this isn’t recommended because:

SafeERC20 is not an ERC20 extension that you use to make your token safe (OpenZeppelin’s ERC20 is already safe). 
It’s a helper to make safe the interaction with someone else’s ERC20 token, in your contracts.

You can read the full post here : https://forum.openzeppelin.com/t/safeerc20-tokentimelock-wrappers/396

Recommended Mitigation Steps

Call the safeTransferFrom() and safeTransfer() method instead of transferFrom() and transfer() for assets transfers

#0 - mejango
2022-07-12T18:39:09Z

dup #281

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xdanial, 0xf15ers, Bnke0x0, Ch_301, Chandr, Chom, Funen, GimelSec, Hawkeye, JC, Kaiziron, Lambda, Meera, MiloTruck, Noah3o6, Picodes, ReyAdmirado, Rohan16, Sm4rty, TerrierLover, TomJ, Waze, _Adam, __141345__, asutorufos, aysha, berndartmueller, brgltd, cccz, codexploder, defsec, delfin454000, djxploit, durianSausage, fatherOfBlocks, hake, horsefacts, hubble, jayfromthe13th, joestakey, jonatascm, m_Rassska, oyc_109, pashov, rajatbeladiya, rbserver, robee, sach1r0, sahar, samruna, simon135, svskaushik, zzzitron

Awards

90.2101 USDC - $90.21

Labels

bug

QA (Quality Assurance)

valid

External Links

Link to GitHub issue

Check Isn't Efficace

Description

allows malicious users to keep calling the _getStructFor () function, via calling the function queuedOf () with parameter _projectId = 0.

Findings

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol

143:    if (latestConfigurationOf[_projectId] == 0) return _getStructFor(0, 0);

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L143

201: if (latestConfigurationOf[_projectId] == 0) return _getStructFor(0, 0); https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L201

Recommended

error INVALID_PROJECTID();
if (latestConfigurationOf[_projectId] == 0)  revert INVALID_PROJECTID();

Unnecessary Return

Description

The function tokenURI returns an empty URI If there's no resolver

Findings

/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBProjects.sol

71:  if (tokenUriResolver == IJBTokenUriResolver(address(0))) return ' ';

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBProjects.sol#L71

Recommended

It is better to use assert() or require() to handle the empty value

assert(tokenUriResolver == IJBTokenUriResolver(address(0)));

Functions With Confused Names

Description

Functions called the same name function from openzeppelin

Findings

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBToken.sol

145:   function approve

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBToken.sol#L145-L153

163: function transfer https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBToken.sol#L163-L171

182: function transferFrom https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBToken.sol#L182-L191

Missing Checks For Address(0x0) When Setting The New Controller

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol 

236:  controllerOf[_projectId] = _controller;

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L236

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol 

339:  isAllowedToSetFirstController[_address] = _flag

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L339

Findings Information

🌟 Selected for report: 0xA5DF

Also found by: 0v3rf10w, 0x09GTO, 0x1f8b, 0x29A, 0xDjango, 0xKitsune, 0xNazgul, 0xdanial, 0xf15ers, Aymen0909, Bnke0x0, Ch_301, Cheeezzyyyy, Chom, ElKu, Funen, Hawkeye, IllIllI, JC, JohnSmith, Kaiziron, Lambda, Limbooo, Meera, Metatron, MiloTruck, Noah3o6, Picodes, Randyyy, RedOneN, ReyAdmirado, Rohan16, Saintcode_, Sm4rty, TomJ, Tomio, Tutturu, UnusualTurtle, Waze, _Adam, __141345__, ajtra, apostle0x01, asutorufos, brgltd, c3phas, cRat1st0s, codexploder, defsec, delfin454000, djxploit, durianSausage, exd0tpy, fatherOfBlocks, hake, horsefacts, ignacio, jayfromthe13th, joestakey, jonatascm, kaden, kebabsec, m_Rassska, mektigboy, mrpathfindr, oyc_109, rajatbeladiya, rbserver, rfa, robee, sach1r0, sashik_eth, simon135

Awards

38.2406 USDC - $38.24

Labels

bug

G (Gas Optimization)

valid

External Links

Link to GitHub issue

Optimal Comparison Operator

Description

When comparing uints, you can save gas costs by strategically picking the optimal operator. In this case, changing the operator >= to > will save more gas, The time is specified in milliseconds so the probability of parity is very low. which is unlikely

Findings

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol 

550:   block.timestamp >= _fundingCycle.start

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L550

589: block.timestamp >= _fundingCycle.start https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L589

593: block.timestamp >= _fundingCycle.start https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L593

602: if (_baseFundingCycle.duration > 0 && block.timestamp >= _baseFundingCycle.start + _baseFundingCycle.duration) return 0; https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L602

815: else if (_ballotFundingCycle.ballot.duration() >= block.timestamp - _configuration) return JBBallotState.Active;

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L815

Calldata Instead Of Memory For Ro Function Parameters

Description

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

It is better to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

Findings

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol 

#406:   JBFundingCycle memory _baseFundingCycle

https://github.com//jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L406

#624: JBFundingCycle memory _baseFundingCycle https://github.com//jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L624

#698: JBFundingCycle memory _baseFundingCycle

https://github.com//jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L698

#770: JBFundingCycle memory _fundingCycle

https://github.com//jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L770

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol

#841:    string memory _memo

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol#L841

Recommended

Replace memory with calldata

Using Prefix Increments Save Gas

Description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

Findings

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol

724: for (uint256 i = 0; i < _discountMultiple; i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBFundingCycleStore.sol#L724

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol

204: for (uint256 _i = 0; _i < _currentSplits.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L204

211: for (uint256 _j = 0; _j < _splits.length; _j++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L211

229: for (uint256 _i = 0; _i < _splits.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L229

304: for (uint256 _i = 0; _i < _splitCount; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L304

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol

85:  for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L85

135: for (uint256 _i = 0; _i < _operatorData.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L135

165: for (uint256 _i = 0; _i < _indexes.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L165

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol 

139: for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L139

167: for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L167

275:   for (uint256 _i; _i < _terminals.length; _i++)
276:   for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L275-L276

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol

913:  for (uint256 _i = 0; _i < _splits.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol#L913

1014: for (uint256 _i; _i < _fundAccessConstraints.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol#L1014

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol

862:  for (uint256 _i = 0; _i < _terminals.length; _i++)

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol#L862

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol

466:   for (uint256 i = 0; i < _splits.length; i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L466

<array>.length Should Not Be Looked Up In Every Loop Of A For-loop

description The code can be optimized by minimizing the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

Findings

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol

204: for (uint256 _i = 0; _i < _currentSplits.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L204

211: for (uint256 _j = 0; _j < _splits.length; _j++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L211

229: for (uint256 _i = 0; _i < _splits.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSplitsStore.sol#L229

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol

85:  for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L85

135: for (uint256 _i = 0; _i < _operatorData.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L135

165: for (uint256 _i = 0; _i < _indexes.length; _i++) { https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBOperatorStore.sol#L165

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol 

139: for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) {

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L139

167: for (uint256 _i; _i < _terminalsOf[_projectId].length; _i++) https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L167

275:   for (uint256 _i; _i < _terminals.length; _i++)
276:   for (uint256 _j = _i + 1; _j < _terminals.length; _j++)

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBDirectory.sol#L275-L276

File:  /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol

913:  for (uint256 _i = 0; _i < _splits.length; _i++) {
```

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol#L913

`1014: for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {`

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBController.sol#L1014

```
File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol

862:  for (uint256 _i = 0; _i < _terminals.length; _i++)
```

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBSingleTokenPaymentTerminalStore.sol#L862

```
File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol

466:   for (uint256 i = 0; i < _splits.length; i++) {
```
https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L466

# Using > 0 Costs More Gas Than != 0
```
File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol

477:  if (_splitAmount > 0) {
```
https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L477

# <X> = <X> - <Y> Costs More Gas Than <X> - = <Y> 
```
File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol

543: leftoverAmount = leftoverAmount - _splitAmount;
```
https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L543

 # Declare This Variable “uint256 _splitAmount” Outside The Loop

File: /jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol

471: uint256 _splitAmount = PRBMath.mulDiv()

https://github.com/jbx-protocol/juice-contracts-v2/blob/main/contracts/JBETHERC20SplitsPayer.sol#L471-L475

Juicebox V2 contest - Ch_301's results

The decentralized fundraising and treasury protocol.

General Information

Findings Distribution

Researcher Performance

External Links

[M-03] Use Safetransferfrom & Safetransfer Instead Of Transferfrom & Transfer For Erc20 Transfers - $3.41

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Details & Impact

Recommended Mitigation Steps

#0 - mejango2022-07-12T18:39:09Z

[Q-30] QA Report - $90.21

Findings Information

Awards

Labels

External Links

Check Isn't Efficace

Unnecessary Return

Functions With Confused Names

Missing Checks For Address(0x0) When Setting The New Controller

[G-61] Gas Report - $38.24

Findings Information

Awards

Labels

External Links

Optimal Comparison Operator

Calldata Instead Of Memory For Ro Function Parameters

Using Prefix Increments Save Gas

<array>.length Should Not Be Looked Up In Every Loop Of A For-loop

#0 - mejango
2022-07-12T18:39:09Z