Juicebox V2 contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Incorrect of stale oracle results

Recommended Mitigation Steps

Change from this: (, int256 _price, , , ) = feed.latestRoundData()

To this: (uint80 roundID, int256 _price, , uint256 timestamp, uint80 answeredInRound) = _feed.latestRoundData(); require(answeredInRound >= roundID, "Stale price"); require(timestamp != 0,"Round not complete"); require(answer > 0,"Chainlink answer reporting 0");

Lines of code

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

Vulnerability details

Some ERC20 tokens don't revert when a transfer fails. In such cases transfer and transferFrom are not reliable to judge if tokens are actually transferred

Tools Used

Recommended Mitigation Steps

Implement OZ's safeERC20 and used safeTransfer and safeTransferFrom for ERC20 transfers

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1440-L1452

Vulnerability details

When feeDiscount isn't 0 or JBConstants.MAX_FEE_DISCOUNT, the fee calculation will be wrong. It always errors to the underside i.e. takes less fees than it should but the discount is not working as intended for the conditions stated.

Impact

Incorrect fee

Proof of Concept

Constants: JBConstants.MAX_FEE_DISCOUNT = 1E9 JBConstants.MAX_FEE = 1E9

Assumptions: _amount = 1E18 _fee = 2.5E7 (standard fee for a terminal) _feeDiscount = 5E8 (50% discount)

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1446-L1447

_discountedFee = 2.5E7 - (2.5E7*5E8/1E9) = 1.25E7

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1451

1E18 - 1E18*1E9/(1.25E7+1E9) = 0.0123E18

0.0123E18 / 1E18 = 1.23% fee

The expected fee would be 1.25% (50% of 2.5%)

Tools Used

Recommended Mitigation Steps

Change L1457-1458 to:

return _amount - PRBMath.mulDiv(_amount, JBConstants.MAX_FEE - discountedFee, JBConstants.MAX_FEE);

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L573-L611

Vulnerability details

Impact

Fees will be permanently stuck

Proof of Concept

Each time a fee is taken, a new entry is pushed to _heldFeesOf[_projectID] in L1199 of takeFeeFrom

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1188-L1208

processFees requires that all outstanding fees be processed in a single transaction. If fees go too long before being collected then this could potentially lead to an array that is too large to process, because _heldFeesOf[_projectID] has no limit in size.

Tools Used

Recommended Mitigation Steps

When storing fees, compare the the fee, feeDiscount and beneficiary of the most recent entry. If all three match (i.e. the fee, feeDiscount and beneficiary haven't changed since last fee collection) then add _amount to the amount of last entry instead of pushing a new one.

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L109-L122

Vulnerability details

Impact

Potentially inconsistent currency conversions

Proof of Concept

addFeedFor requires that a price feed for the _currency _base doesn't exist when adding a new price feed but doesn't check if the inverse already exists. This means that two different oracles (potentially with different prices) could be used for _currency -> _base vs. _base -> _currency. Different prices would lead to inconsistent between conversion ratios depending on the direction of the conversion

Tools Used

Recommended Mitigation Steps

Change L115 to: if (feedFor[_currency][_base] != IJBPriceFeed(address(0)) || feedFor[_base][_currency] != IJBPriceFeed(address(0))) revert PRICE_FEED_ALREADY_EXISTS()