Juicebox V2 contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

Consumers of a Chainlink price feed should always verify that the price they receive is fresh, i.e. submitted recently. Otherwise, your protocol might work with outdated price data.

In the recent incident with Luna/UST, we've seen a protocol that doesn't verify the Chainlink data resulting in them losing ~$11M: https://medium.com/@blizzfinance/blizz-finance-post-mortem-2425a33fe28b

Proof of Concept

The contract only retrieves the price and ignores all the other values:

(, int256 price, , , ) = feed.latestRoundData();

Tools Used

none

Recommended Mitigation Steps

Verify that the data is fresh by checking the updatedAt parameter: https://docs.chain.link/docs/price-feeds-api-reference/#latestrounddata

Also, Chainlink recommends consumers be able to pause usage of a specific feed in case the feed is paused: https://docs.chain.link/docs/selecting-data-feeds/#risk-mitigation

Lines of code

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

Vulnerability details

Impact

Some tokens don't follow the ERC20 standard. For example, some don't revert on failure but instead just return false. With your current implementation, you won't catch these cases. Your contract will continue the transaction believing that the transfer was successful. Here's a good repo on that: https://github.com/d-xo/weird-erc20

Because of that, it's recommended to use OpenZeppelin's SafeERC20 library.

Proof of Concept

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

Tools Used

none

Recommended Mitigation Steps

Use SafeERC20.

Lines of code

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

Vulnerability details

Impact

The fee calculation seems to be wrong. The formula you use is: $x - x * MaxFee / (fee + MaxFee)$. But instead you should use $x * fee / MaxFee$.

Proof of Concept

Using the default values: $x = 100,000,000,000$ $fee = 25,000,000$ $MaxFee = 100,000,000,000$ You take: $100,000,000,000 - 100,000,000,000 * 1,000,000,000 / 1,025,000,000 = 2439024390.24$ instead of: $100,000,000,000 * 25,000,000 / 1,000,000,000 = 2500000000$

Tools Used

none

Recommended Mitigation Steps

Use the other formula to compute the fees:

return PRBMath.mulDiv(_amount, _discountedFee, JBConstants.MAX_FEE);