Juicebox V2 contest - apostle0x01'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: 79/105

Findings: 2

Award: $41.64

🌟 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-code4rena/tree/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L88 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/tree/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L271 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/tree/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L315 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/tree/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L256 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/tree/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L348

Vulnerability details

Unchecked return value for transferFrom calls

Impact - Medium

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference

Similar medium severity issue from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

Poc

There are several transferFrom calls that do not check the return value (some tokens signal failure by returning false):

./contracts/JBERC20PaymentTerminal.sol:88:      : IERC20(token).transferFrom(_from, _to, _amount);
./contracts/JBETHERC20ProjectPayer.sol:271:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20ProjectPayer.sol:315:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20SplitsPayer.sol:256:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);
./contracts/JBETHERC20SplitsPayer.sol:348:      IERC20(_token).transferFrom(msg.sender, address(this), _amount);

Recommendation

It is usually good to add a require-statement that checks the return value or to use something like safeTransferFrom; unless one is sure the given token reverts in case of a failure.

#0 - mejango
2022-07-12T17:36:44Z

dup of #281

#1 - drgorillamd
2022-07-12T20:43:02Z

dup of https://github.com/code-423n4/2022-07-juicebox-findings/issues/281

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.2308 USDC - $38.23

Labels

bug

G (Gas Optimization)

old-submission-method

valid

External Links

Link to GitHub issue

[G-01] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:594:    for (uint256 _i = 0; _i < _heldFeeLength; ) {
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
./contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol:1396:    for (uint256 _i = 0; _i < _heldFeesLength; ) {
./contracts/JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
./contracts/JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; i++) {
./contracts/JBFundingCycleStore.sol:724:    for (uint256 i = 0; i < _discountMultiple; i++) {
./contracts/JBOperatorStore.sol:85:    for (uint256 _i = 0; _i < _permissionIndexes.length; _i++) {
./contracts/JBOperatorStore.sol:135:    for (uint256 _i = 0; _i < _operatorData.length; _i++) {
./contracts/JBOperatorStore.sol:165:    for (uint256 _i = 0; _i < _indexes.length; _i++) {
./contracts/JBSingleTokenPaymentTerminalStore.sol:862:    for (uint256 _i = 0; _i < _terminals.length; _i++)
./contracts/JBSplitsStore.sol:165:    for (uint256 _i = 0; _i < _groupedSplitsLength; ) {
./contracts/JBSplitsStore.sol:204:    for (uint256 _i = 0; _i < _currentSplits.length; _i++) {
./contracts/JBSplitsStore.sol:211:      for (uint256 _j = 0; _j < _splits.length; _j++) {
./contracts/JBSplitsStore.sol:229:    for (uint256 _i = 0; _i < _splits.length; _i++) {
./contracts/JBSplitsStore.sol:304:    for (uint256 _i = 0; _i < _splitCount; _i++) {

Mitigation

As an example: for for (uint256 _i = 0; _i < _heldFeeLength; ) should be replaced with for for (uint256 _i ; _i < _heldFeeLength; ){

[G-02] For-Loops

Impact

An array's length should be cached to save gas in for-loops Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

abstract/JBPayoutRedemptionPaymentTerminal.sol:1008:    for (uint256 _i = 0; _i < _splits.length; ) {
JBController.sol:913:    for (uint256 _i = 0; _i < _splits.length; _i++) {
JBController.sol:1014:    for (uint256 _i; _i < _fundAccessConstraints.length; _i++) {
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++)
JBETHERC20SplitsPayer.sol:466:    for (uint256 i = 0; i < _splits.length; 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++) {
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++) {

Juicebox V2 contest - apostle0x01's results

The decentralized fundraising and treasury protocol.

General Information

Findings Distribution

Researcher Performance

External Links

[M-03] Unchecked return value for transferFrom calls - $3.41

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Unchecked return value for transferFrom calls

Impact - Medium

Reference

Poc

Recommendation

#0 - mejango2022-07-12T17:36:44Z

#1 - drgorillamd2022-07-12T20:43:02Z

[G-72] Gas Report - $38.23

Findings Information

Awards

Labels

External Links

[G-01] No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

POC

Mitigation

[G-02] For-Loops

Impact

#0 - mejango
2022-07-12T17:36:44Z

#1 - drgorillamd
2022-07-12T20:43:02Z