Excess ETH when swapping tokens is not returned back and will be stuck forever on the JBXBuybackDelegate contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

All the excess ETH send to make swaps on the JBXBuybackDelegate contract will be stuck forever inside the contract.

Proof of Concept

• The issue is caused because when doing a swap(), there is no a track between how much ETH was really paid and how much ETH was sent.
  • The logic assumes that the _amountToSend calculated by the Uniswap pool will take a 100% of the sent ETH, which might not be always the case.

Coded a PoC to demonstrate this issue

• For the purpose of this PoC I'll need your help to add this view function on the JBXBuybackDelegate contract, just to facilitate the monitoring of the ETH balance that the contract is holding, both, before and after doing a swap.

1. Add the below view function to the JBXBuybackDelegate contract

function buyBackDelegateBalance() external view returns (uint256) {
    return address(this).balance;
}

2. Now is time to make a slightly update to the testDatasourceDelegateSwapIfPreferenceIsToClaimTokens() in the DelegateUnit.t.sol test file, basically, we need to call the buyBackDelegateBalance() function at the beggining and at the end of the test.

function testDatasourceDelegateSwapIfPreferenceIsToClaimTokens() public {

+   // Check the balanceOf the JBXBuybackDelegate.sol contract before the calls!
+   uint256 buyBackDelegateBalanceBefore = _delegate.buyBackDelegateBalance();

    ...
    ...
    ...

+   // Check the balanceOf the JBXBuybackDelegate.sol contract after the calls!
+   uint256 buyBackDelegateBalanceAfter = _delegate.buyBackDelegateBalance();
}

• The output will be something similar to the below logs, which as we can see, all the unspend ETH is left on the JBXBuybackDelegate contract.
• NOTE: It is important to mention that on this PoC all the ETH that was sent to JBETHPaymentTerminal::pay() is left on the JBXBuybackDelegate contract, the reason is because in this PoC there is no an actual payment of WETH tokens to the uni pool, but nevertheless, when the real swaps are executed, the ETH that will be left on the contract will be the difference between the msg.value sent to JBETHPaymentTerminal::pay() and the _amountToSend calculated by Uniswap.

Running 1 test for contracts/test/DelegateUnit.t.sol:TestUnitJBXBuybackDelegate
[PASS] testDatasourceDelegateSwapIfPreferenceIsToClaimTokens() (gas: 267256)
Traces:
  [267256] TestUnitJBXBuybackDelegate::testDatasourceDelegateSwapIfPreferenceIsToClaimTokens()
    ├─ [290] JBXBuybackDelegate::buyBackDelegateBalance() [staticcall]
    │   └─ ← 0
    ├─ [0] VM::prank(projectOwner: [0x000000000000000000000000000000000000007B])

   ...
   ...
   ...

    ├─ [290] JBXBuybackDelegate::buyBackDelegateBalance() [staticcall]
    │   └─ ← 1000000000000000000
    └─ ← ()

Take as reference the logic on the _mint() function that succesfully returns back the received ETH back to the PaymentTerminal contract

Tools Used

Manual Audit

Recommended Mitigation Steps

• Calculate the difference between how much ETH was really paid for the swap (_amountToSend), and deduct it from the total msg.value that was send to jbETHPaymentTerminal().pay()
• Make sure to return back the difference to either the user or the PaymentTerminal, if any ETH is left on the JBXBuybackDelegate contract it will be stuck forever since there is no a function to withdraw it to another address.

Assessed type

Other

[c4-pre-sort]

dmvt marked the issue as duplicate of #42

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as satisfactory