`_minReturnedTokens` in `pay()` will always have to be set to 0 which in some cases can result in unexpected return for the payer.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L166 https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/JBSingleTokenPaymentTerminalStore.sol#L409 https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1385 https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/test/DelegateUnit.t.sol#L193

Vulnerability details

Impact

Why does _minReturnedTokens = 0 :

There is a check in the _pay() function of JBPayoutRedemptionPaymentTerminal.sol, that requires beneficiaryTokenCount to be higher or equal than _minReturnedTokens (https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/abstract/JBPayoutRedemptionPaymentTerminal.sol#L1373-L1385C11).

JuiceBoxBuybackDelegate can return weight = 0 in payParams() if the swap path is chosen over the minting.

If using it as a delegate and data source, when weight = 0 then recordPaymentFrom() will return tokenCount = 0 which will then cause beneficiaryTokenCount to be 0.

See: https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L166 and https://github.com/jbx-protocol/juice-contracts-v3/blob/12d852f28d372dd44987586f8009c56b0fe247a9/contracts/JBSingleTokenPaymentTerminalStore.sol#L409.

Because of that the only way to pass this check is to use _minReturnedTokens = 0

This is validated by the tests files (https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/test/DelegateUnit.t.sol#L193).

Why is it an issue :

_minReturnedTokens is here to make sure the user will always get a minimum return when calling pay().

Because the returns on user's investement are determined by the cycle params, if these params where to change while the user transaction is in the MEM pool, the returns would get impacted.

Fortunately it seems like cycle's params are fixed for the cycle duration, protecting the user in most cases.

But if a cycle was to end and a new one to start while the transaction is being mined, the returns for the user could change if the new cycle's params are different.

Proof of Concept

• A user submit a pay() TX that end up minting and not swapping on a cycle about to end assuming current cycle params (weight, reserve etc..).
• Gas price rise by a lot because a new NFT being claimed and last few hours.
• The TX gets mined few hours later but a new cycle started, this new cycle has new params (ex:a new token weight). The received token amount will differ from what the user was originally expecting.

Tools Used

Manual review

Recommended Mitigation Steps

A _minReturnedTokensDelegate variable could be passed in the metadata variable and then be checked in the JuiceBoxBuybackDelegate contract and revert if tokenCount is inferior.

Assessed type

Timing

[c4-pre-sort]

dmvt marked the issue as primary issue

[simon-something]

2 things:

• _minReturnedToken is not really used currently in the protocol, as it's rather for future-proof (for the day where we have terminals with various currencies, where a slippage might appear) - screenshot of the latest pay(..) for instance:

• in this setting, the delegate doesn't receive this arg (hence the use of the "free" metadata arg, to pass the quote/min received)

The scenario of "the transaction is pending then current fc rolls over and the beneficiary receive less token" looks a bit super edge case (esp for a med risk)...

Agreed it should be more documented tho, as it might be confusing rn

[c4-sponsor]

drgorillamd marked the issue as disagree with severity

[c4-judge]

dmvt marked issue #232 as primary and marked this issue as a duplicate of 232

[dmvt]

I'm going to leave this in place as a medium. The code functions in unexpected ways based on external factors and it's actual function is very different than a user could reasonably expect. If the variable should always be zero, consider removing it. I don't agree with the assertion that including dead arguments which may be used in the future is future-proof.

Also, see here wrt protecting users from themselves.

[c4-judge]

dmvt marked the issue as satisfactory