Usage of mintedAmount and reservedRate across two function is vulnerable

[code423n4]

Lines of code

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#L183-L209

Vulnerability details

Impact

Detailed description of the impact of this finding. mintedAmount and reservedRate are two state variables that are initialised to 1. These two variables are being used as channel of information across two functions payParams() and didPay(). Both are external functions and hence can be called by anyone.

The payParams() set the value of mintedAmount and reservedRate based on input data.

 if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
            // Pass the quote and reserve rate via a mutex
            mintedAmount = _tokenCount;
            reservedRate = _data.reservedRate;

and didPay() consumes these values and resets them back to 1.

        uint256 _tokenCount = mintedAmount;
        mintedAmount = 1;

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

Here, there is a potential where before the didPay() function is executed, the payParams() function is called to set these two variables to a different value set. As the intention is to use the two pair functions together, there should be a mechanism to prevent another call change the state in between.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

// Add a new storage variable bool internal locked;

modifier NotInUse() { require(!locked, "Call in use"); _; }

attach the above modifier to payParams() so that it will allow only one participant to enter. In the payParams(), set the locked to true.

in didPay(), set the locked to false.

Tools Used

Recommended Mitigation Steps

Implement the above proposal of locking using a bool as storage variable to ensure that there is only one participant interacting with the contract.

Assessed type

Other

[c4-pre-sort]

dmvt marked the issue as duplicate of #60

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid