Juicebox V2 contest - cryptphi's results

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#L555 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L772 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L885 https://github.com/jbx-protocol/juice-contracts-v2-code4rena/blob/828bf2f3e719873daa08081cfa0d0a6deaa5ace5/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L965

Vulnerability details

Impact

There are multiple external calls to IERC20.transfer() and IERC20.transfer() in contracts that suppose to transfer out tokens from/to the system. However, there are tokens like USDT that don't return any return value in their implementation of the transfer() function, so calling IERC20.transfer() on them will cause the transaction a revert (a kind of DoS - the funds are stuck at the contract).

In addition, tokens which do return a return value in their implementation of the transfer() function should return whether the transfer succeeded of failed, but that return value is not checked in the contract. This can lead to the caller not being debited when calling the pay function or could lead to false events

Multiple functions in JBPayoutRedemptionPaymentTerminal contract are affected due to the use of the unsafe transfer or transferFrom call inherited from JBERC20PaymentTerminal contract

Proof of Concept

JBERC20PaymentTerminal._transferFrom() makes the unsafe transfer() or transferFrom() calls, this internal function are affects JBPayoutRedemptionPaymentTerminal contract in the following functions: pay() addToBalanceOf() _redeemTokensOf() _distributePayoutsOf() _useAllowanceOf()

Tools Used

Manual review

Recommended Mitigation Steps

Use OpenZeppelin's SafeERC20.safeTransfer() and SafeERC20.safeTransfer() functions instead.