Juicebox Buyback Delegate - lfzkoala's results

#Issue 1 DoS with Unexpected Throw.

If the _data.metadata array in payParams and didPay is not the expected length, the function call to abi.decode() will throw an exception, leading to denial of service (DoS). To mitigate this issue, consider using try-catch around it.

Issue 2 Front Running.

In the payParams function, the calculated _tokenCount is compared with _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR), where _quote and _slippage are extracted from the metadata of the passed _data object. Suppose a legitimate user intends to use this function and puts an _amount of their tokens and their desired _quote and _slippage in the metadata. They then submit the transaction. A malicious observer (the front-runner) sees this pending transaction in the transaction pool. They can inspect the details and see the _quote and _slippage the original user is aiming for. They could then craft a transaction that undercuts the original user, for instance by setting a slightly better _quote and _slippage, and they broadcast this transaction with a higher gas fee. Because miners are incentivized by gas fees, they will likely include the front-runner's transaction in the block before the original user's transaction. This allows the front-runner to benefit from the price setup that the original user intended for themselves. To help mitigate such attacks, developers can consider implementing commit-reveal schemes or threshold cryptography. Alternatively, a less complex approach would be to include time-lock mechanisms or use batch auctions. However, all these solutions have their own trade-offs and complexities.

Issue 3 Lack of Input Validation.

Description: The function didPay() doesn't perform any validation on the data it receives via its arguments. For instance, there's no validation on _data.amount.value, _data.metadata, etc.

Proof of Concept: Malformed or maliciously crafted input data can cause the function to behave unexpectedly.

Code Snippet:

(,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);
...
uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);

Resolution: Include checks to validate the data received. Ensure that _data.amount.value is non-zero, _data.metadata can be correctly decoded, etc. For example, you might want to add something like require(_data.amount.value > 0, "Amount should be greater than zero").

Code Example:

require(_data.amount.value > 0, "Amount should be greater than zero");
// Decode the metadata and validate
(,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
require(_quote >= _slippage, "Quote should be greater than or equal to slippage");
...
uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);

Issue 4 Potential Underflow

Description: In function didPay(), the _minimumReceivedFromSwap calculation could potentially underflow if _slippage is greater than _quote.

Proof of Concept:

uint256 _quote = 5;
uint256 _slippage = 10;
uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR); // Underflow

**Code Snippet:

**

(,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);

Resolution: Validate that _slippage is less than or equal to _quote to prevent potential underflow.

Code Example:

(,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
require(_slippage <= _quote, "Slippage should be less than or equal to quote");
uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);

Issue 5 Potential Front-Running Attack

Description: The _swap function is vulnerable to front-running attacks due to the use of public data to make key calculations, such as the _minimumReceivedFromSwap.

Proof of Concept: A malicious miner could inspect pending transactions, spot a transaction calling the _swap function, then quickly submit their own transaction with a higher gas price. The miner's transaction is likely to be confirmed first due to the higher gas price, which could lead them to gain more favorable swap terms.

Code Snippet:

// In _swap function
uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);
...
try pool.swap({...}) returns (int256 amount0, int256 amount1) {...}

Resolution: Using private or off-chain calculations may reduce the risk of front-running attacks. Also, keep in mind that not all types of front-running attacks can be prevented, especially those that involve miners.

Issue 6 Handling of Failed Contract Calls

Description: The _swap function does not handle failed calls to controller.burnTokensOf and controller.mintTokensOf. If these calls fail (for example, due to the receiving contract throwing an exception), the contract will continue execution as if they were successful.

Proof of Concept: Any failure in burnTokensOf and mintTokensOf will not stop the contract from executing further.

Code Snippet:

// In _swap function
controller.burnTokensOf({...});
controller.mintTokensOf({...});

Resolution: Always check the return value of function calls, if any, which should indicate if the operation was successful. If there's no return value, consider using Solidity's low-level call function, which returns false if the call fails.

Code Example:

(bool success,) = address(controller).call(abi.encodeWithSignature("burnTokensOf(address,uint256)", {...}));
require(success, "Burn tokens failed");
(success,) = address(controller).call(abi.encodeWithSignature("mintTokensOf(uint256,uint256,address,string,bool,bool)", {...}));
require(success, "Mint tokens failed");

Issue 7 Incomplete Function Implementation

Description: The function redeemParams has been declared but its implementation is empty. This could potentially cause unexpected behaviors and/or failures when the function is invoked.

Proof of Concept: Any call to redeemParams will result in no operation, which could cause issues if this function is supposed to perform any specific tasks.

Location of the Security Issue:

function redeemParams(JBRedeemParamsData calldata _data)
     external
     override
     returns (uint256 reclaimAmount, string memory memo, JBRedemptionDelegateAllocation[] memory delegateAllocations)
 {}

Resolution: Implement the function redeemParams according to the expected business logic or remove it if it is not necessary.