Unsafe call to `ERC20::transfer` can result in stuck funds

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L232

Vulnerability details

Impact

In the uniswapV3SwapCallback method we have the following code for transferring ERC20 tokens

weth.transfer(address(pool), _amountToSend);

The problem is that the transfer function from ERC20 returns a bool to indicate if the transfer was a success or not. As there are some tokens that do not revert on failure but instead return false (one such example is ZRX) and also Zerem should work with all types of ERC20 tokens since it might be integrated with a protocol that does that, not checking the return value can result in tokens getting stuck.

If an ERC20::transfer call fails it will lead to stuck funds for a user. This only happens with a special class of ERC20 tokens though, so it is Medium severity.

Proof of Concept

Let’s look at the following scenario:

1. Alice is trying to claim some tokens from a protocol that has integrated with Zerem, so her transaction makes a call to Zerem::transferTo
2. The amount to claim is less than the lockThreshold so the code goes to the_sendFunds functionality directly
3. The transfer fails and since the token does not revert but returns false this is not accounted for by the Zerem protocol and transaction completes successfully
4. The tokens are now stuck and are not claimable by Alice anymore

Tools Used

Manual

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20 library and change transfer to safeTransfer. Fixed by adding SafeERC20

Assessed type

Token-Transfer

[c4-pre-sort]

dmvt marked the issue as low quality report

[dmvt]

spam / bot race

[c4-judge]

dmvt marked the issue as unsatisfactory: Out of scope