Juicebox V2 contest - cloudjunky's results

Lines of code

https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBERC20PaymentTerminal.sol#L99 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L364 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20ProjectPayer.sol#L412 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/JBETHERC20SplitsPayer.sol#L493

Vulnerability details

Issue

The .approve() call is the sole responsibility of the _beforeTransferTo() function which is called in JBERC20PaymentTerminal.sol and multiple times in JBPayoutRedemptionPaymentTerminal.sol.

There are some examples where .approve() is used but the return value is checked;

1. L99 in JBERC20PaymentTerminal.sol uses an unsafe approve() function and doesn’t check the return value or revert if not true.
2. L364 in JBETHERC20ProjectPayer.sol uses an unsafe approve() function and doesn’t check the return value or revert if not true.
3. L412 in JBETHERC20ProjectPayer.sol uses an unsafe approve() function and doesn’t check the return value or revert if not true.
4. L493 in JBETHERC20SplitsPayer.sol uses an unsafe approve() function and doesn’t check the return value or revert if not true.

Proof of Concept

Some ERC20 tokens return false and because the return value is not checked subsequent transfers may fail. The contract can enter an unknown state - inferring that approvals have taken place in error.

Furthermore .approve() is unsafe as it can be front-run in specific scenarios.

Tools Used

Vim

Recommended mitigation steps

A better alternative would be to use Openzeppelin’s SafeERC20 library safe variants .safeIncreaseAllowance() and .safeDecreaseAllowance(). Please do not use .safeApprove() as it has been deprecated.