Juicebox V2 contest - Franfran's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBPrices.sol#L69 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBChainlinkV3PriceFeed.sol#L44 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBSingleTokenPaymentTerminalStore.sol#L385-L387

Vulnerability details

Impact

The price feed could send wrong values because the round could still be ongoing, as the timestamp is not checked. As a result, an unfair amount of tokens could be minted from the payment terminal store.

Proof of Concept

https://docs.chain.link/docs/historical-price-data/#getrounddata-return-values

Tools Used

vim

Recommended Mitigation Steps

Consider doing:

(, int256 price,, uint256 timestamp,) = feed.latestRoundData();
require(timestamp != 0 ,"Round is still ongoing");
require(price > 0, "Incorrect price");

Lines of code

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

Vulnerability details

Impact

As explained in the EIP20 "Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!" In the first link (core contract handling those transfers), the status of the transfer is not returned. As a result, (2nd link) the _pay internal function is called and some state are updated, while the transfer may have silently failed before. e.g. Some extra tokens could be minted (link 3).

Proof of Concept

Tools Used

foundry

Recommended Mitigation Steps

Return the transfer state from the _transferFrom() and use require statement from calling contracts. You may also want to wrap this require in the JBERC20PaymentTerminal.sol to avoid future errors linked to this.