code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

High impact because mintedAmount and reservedRate are used to assign new variable values in didPay(), which directly affects the amount of tokens minted.

Proof of Concept

Anyone can call payParams() and pass values which are decoded like -

(,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));

also the value of _tokenCount is set using _data.amount.value and _data.weight

uint256 _tokenCount = PRBMath.mulDiv(_data.amount.value, _data.weight, 10 ** 18);

By passing a a very high value in slippage (uint256), it seems possible to enter this if condition, and set reservedRate and mintedAmount.

 // 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);
        }

Since this function is public, and each listed project will have this single contract with which all users will interact, its possible for anyone to manipulate these values and ruin correct functioning of didPay() where these variable values are utilized.

Below code is from didPay() where _tokenCount and _reservedRate is set from those values.

// Retrieve the number of token created if minting and reset the mutex (not exposed in JBDidPayData)
        uint256 _tokenCount = mintedAmount;
        mintedAmount = 1;

        // Retrieve the fc reserved rate and reset the mutex
        uint256 _reservedRate = reservedRate;
        reservedRate = 1;

Tools Used

Manual

Recommended Mitigation Steps

I don't think common storage variables should be allowed to be changed by anyone. Or at least they should not be used this way in didPay() function.

Assessed type

Other

c4-pre-sort commented 1 year ago

dmvt marked the issue as duplicate of #60

c4-judge commented 1 year ago

dmvt changed the severity to 2 (Med Risk)