Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L286 https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L144-L171 https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L263-L271

Vulnerability details

Impact

In the JBXBuybackDelegate._swap() function there is a possbile reentrancy vulnerability. If the projectToken is a ERC777 token then the _data.beneficiary can reenter the contract by calling the JBXBuybackDelegate.payParams() external function to mint more project tokens for itself.

ERC777 tokens have hooks implemented for transfer, burn and mint. This is problemtic with ERC777 since it is backward compatible with ERC20 tokens.

If the projectToken happens to be a ERC777 token then it can be considered as a ERC20 token as well due to backward compatibility. In the JBXBuybackDelegate._swap() function, when the projectToken.transfer() is called on the token, it will call on the tokensReceived hook of the _data.beneficiary. If the _data.beneficiary is a malicious contract, it can reenter the JBXBuybackDelegate contract.

Same condition applies in the JBXBuybackDelegate._mint() function as well. Because the mint() function is called on the projectToken. If projectToken is ERC777, then the tokensReceived hook will be called on the _data.beneficiary contract which is the reciepient of the newly minted tokens. If the _data.beneficiary is a malicious contract, it can reenter the JBXBuybackDelegate contract.

Consider the following scenario:

The attacker calls the JBXBuybackDelegate contract and enters the JBXBuybackDelegate._swap() function (given that swap quote is higher), with _data.amount.value = 0 value.
There is no input validation to check for _data.amount.value == 0 and revert the transaction.
Hence the JBXBuybackDelegate._swap() function will be called in case of the swap, which in turn will call the pool.swap() function with _data.amount.value = 0 value.
The pool.swap() will return _amountReceived = 0 without reverting the transaction, since there was no tokens to be swapped.
Now the _amountReceived = 0 without any exception in the pool.swap() function call, Hence the transaction will not return in the catch statement.
The transaction will proceed to the projectToken.transfer call and will try to transfer zero amount of _nonReservedToken.
Still the transaction will not revert since the transfer function does not check for zero amount and revert.
This will try to transfer _nonReservedToken which is zero to the `_data.beneficiary which is a malicious contract setup by the attacker.
The malicious contract (_data.beneficiary) will use the tokensReceived hook to reenter the JBXBuybackDelegate contract.
It will call the JBXBuybackDelegate.payParams() external function with a manipulated JBPayParamsData calldata _data input.
Since JBXBuybackDelegate.payParams() is an external function and there is no access control, the _data.beneficiary can easily enter this function.
In the _data parameter, the _data.amount.value can be manipulated to a different value.
This will recalculate the _tokenCount to a manipulated value.
Hence mintedAmount which is a state variable can now be set to a different value.
Similarly reservedRate state variable can be changed by manipulating the _data.reservedRate to a different value.
Hence the state of the contract has been changed in the middle of the transaction. At this stage of the transaction the mintedAmount and reservedRate were expected to be reset to 1. But now the two state variables have been set to two different values manipulated by the attacker.
Hence the contract has entered a state that was not intended in the middle of the transaction.
Now the transaction will return to the JBXBuybackDelegate._swap() and continue the rest of the function call with _amountReceived = 0 value. There won't be any revert since there is no check for zero value within the function.
The JBXBuybackDelegate._swap() function will finish executing, and transaction will retrun to JBXBuybackDelegate.didPay() function.
Now since the _amountReceived == 0 the following if statement will execute.
```
if (_amountReceived == 0) _mint(_data, _tokenCount);
```
This will call on the _mint() function. And the _data.beneficiary will be minted the project tokens.
Even though there is no immediate threat since the manipulated mintedAmount and reservedRate state variables are not used in the transaction after they have been changed, this puts the contract in an unintended state in the middle of the transaction.

Proof of Concept

        if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken);

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

    function payParams(JBPayParamsData calldata _data)
        external
        override
        returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
    {
        // Find the total number of tokens to mint, as a fixed point number with 18 decimals
        uint256 _tokenCount = PRBMath.mulDiv(_data.amount.value, _data.weight, 10 ** 18);

        // Unpack the quote from the pool, given by the frontend
        (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));

        // If the amount swapped is bigger than the lowest received when minting, use the swap pathway
        if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
            // Pass the quote and reserve rate via a mutex
            mintedAmount = _tokenCount;
            reservedRate = _data.reservedRate;

            // Return this delegate as the one to use, and do not mint from the terminal
            delegateAllocations = new JBPayDelegateAllocation[](1);
            delegateAllocations[0] =
                JBPayDelegateAllocation({delegate: IJBPayDelegate(this), amount: _data.amount.value});

            return (0, _data.memo, delegateAllocations);
        }

        // If minting, do not use this as delegate
        return (_data.weight, _data.memo, new JBPayDelegateAllocation[](0));
    }

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L144-L171

        try pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
            data: abi.encode(_minimumReceivedFromSwap)
        }) returns (int256 amount0, int256 amount1) {
            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L263-L271

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended to implement access control in the JBXBuybackDelegate.payParams() function as give below, so only the address(jbxTerminal) can access the function.

    if (msg.sender != address(jbxTerminal)) revert JuiceBuyback_Unauthorized();

Assessed type

Reentrancy

code-423n4 / 2023-05-juicebox-findings

REENTRANCY IN THE ERC777 PROJECT TOKEN, CAN LEAD TO INCONSISTENT STATE OF THE CONTRACT DURING TRANSACTION #252