2023-07-24 – Hold Fees Calculation Error

Author: filipv
Date: 2023-07-24
Severity: Medium
Status: Resolved

On 24th July 2023, a discrepancy in "hold fees" calculations resulted in a minor redemption discrepancy for Legends project supporters. In total, 0.006157049375371805 ETH of held fees were not refunded due to a calculation error in JBPayoutRedemptionPaymentTerminal3_1. This summary details the sequence of events, explains the technical error, and notes measures taken for mitigation and remediation.

Sequence of Events

1. To raise funds for an auction, 🧠🧠🧠.eth deployed the Legends project with an unlimited payout to the project owner and "hold fees" enabled.
2. The project received 12.35 ETH.
3. 10.097560975609756097 ETH was paid out to 🧠🧠🧠.eth.
4. The project received an additional 1 ETH.
5. 10.097560975609756 ETH was returned to the project (only 97 wei less than what had been paid out).
6. The project only had an overflow of 11.343842950624628098 ETH when it should have had 11.35 ETH (minus 97 wei) of overflow.

Because of this, the project's supporters did not receive a full refund when redeeming their tokens. As demonstrated by this simulation, the remaining 0.006157049375371805 ETH are still being held as fees.

Explanation

The terminal represents held fees as a mapping:

/// @notice Fees that are being held to be processed later.
/// @custom:param _projectId The ID of the project for which fees are being held.
mapping(uint256 => JBFee[]) internal _heldFeesOf;

If a project has "hold fees" enabled, _takeFeeFrom adds a JBFee struct to its _heldFeesOf mapping:

_heldFeesOf[_projectId].push(JBFee(_amount, uint32(fee), uint32(_feeDiscount), _beneficiary));

Note that the _amount here is based on the full amount being paid out from the project, including the fee. These held fees can be restored later with _refundHeldFees:

// Process each fee.
for (uint256 _i; _i < _heldFeesLength; ) {
  if (leftoverAmount == 0) _heldFeesOf[_projectId].push(_heldFees[_i]);
  else if (leftoverAmount >= _heldFees[_i].amount) {
    unchecked {
      leftoverAmount = leftoverAmount - _heldFees[_i].amount;

The _refundHeldFees function is invoked when _addToBalanceOf is called with _shouldRefundHeldFees set to true:

uint256 _refundedFees = _shouldRefundHeldFees ? _refundHeldFees(_projectId, _amount) : 0;

But the _amount used when calling _refundHeldFees is based on the amount being returned to the project – not the amount which was taken out. Even if a project creator returns all received funds to a project, it will only refund 97.5% (100% minus the protocol fee) of the amount paid out, as they never received the "fee amount".

Mitigation

Jango has created a pull request to address these issues:

@filipviz and I discovered a bug July 24, 2023 while helping project #548 raise funds for an auction. The project had heldFees on. After failing to win the auction and upon returned the funds, the project was not made whole. The bug is that the protocol expected a deposit equivalent to the amount paid out. In other words, if 10 ETH was paid out (before the fee), the protocol was expected a deposit of 10 ETH to return the full fee amount. The problem is the recipient doesn't have the full 10 ETH, they only have the amount after the fee. The protocol should only expect the a deposit of the amount paid out after fees... the amount the recipient actually has.

This PR introduces JBPayoutRedemptionTerminal3_1_2 that fixes this issue.

It also

• moved fee calculations into a JBFees library
• removes the isTerminalOf check from pay and addToBalance to reduce contract size to be deployable. In pay these checks are already made when minting tokens. Clients are now responsible for making sure this check is correct, otherwise the project can access the funds from the terminal by setting distribution limits in a subsequent cycle.

Remediation

I (filipv) refunded Legends supporters according to their contributions in transaction 0x54cff0ab259f8f10fa562bf6fff0999ed668a5d6b96a23994610a0a33333406e:

Beneficiary	Contribution (ETH)	Refund (ETH)
0xa44533b69f8f21671802cada15b88b3e180b9815	0.001	0.000000542423519987
0x0966d26521c18e82d11c40d64d3d1853ced5e707	1	0.000542423519986944
0x29f088cff86f03036220393c869519329bf69ca7	1.5	0.000813635279980417
0x77cf6c6bf1c57a256847352e2dc513f17757b253	1	0.000542423519986944
0x1bc14df10258bd5fc5bf8717e46037aaaad915eb	0.5	0.000271211759993472
0xbbfaff5990e92852f72c52bd0b71b62593388047	0.5	0.000271211759993472
0x5284b0ba212a1f94246afdf3ceb892441d1e3434	0.1	0.000054242351998694
0x43f3d84aeece98167a6fd8d8361d38351cbe68d0	1	0.000542423519986944
0x798a1edc64727489707c2cfc273d3abd9aff27bb	1	0.000542423519986944
0x1105408dccddbb7ce5d509a5516d4a9a0b6baaeb	1.5	0.000813635279980417
0x3bbd41d9919b8eae59609c8ebfdce48dce2a568a	1	0.000542423519986944
0xd3f9bf35dd1a84975b344a6d6b2a76f61d5c82d6	0.5	0.000271211759993472
0x518d0f7a7432fe475ca36f3eb6a4a7f408a8bd61	0.45	0.000244090583994125
0x1b360b6195450194f077bfeda52a20396ccb07ef	1.3	0.000705150575983028