OperatorProposal.sol: Leftover ETH is not refunded to the msg.sender

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-party/blob/440aafacb0f15d037594cebc85fd471729bcb6d9/contracts/proposals/OperatorProposal.sol#L25-L49

Vulnerability details

Impact

The OperatorProposal contract is a type of proposal that allows to execute operations on contracts that implement the IOperator interface.

Upon execution of the proposal it might be necessary that the executor provides ETH.

This is true especially when allowOperatorsToSpendPartyEth=false, i.e. when ETH cannot be spent from the Party's balance. So it must be provided by the executor.

The amount of ETH that is needed to execute the operation is sent to the operator contract:

Link

data.operator.execute{ value: data.operatorValue }(data.operatorData, executionData);

The operator contract then spends whatever amount of ETH is actually necessary and returns the remaining ETH.

For example the CollectionBatchBuyOperator contract may not spend all of the ETH because the actual purchases that are made are not necessarily known at the time the proposal is created. Also not all purchases may succeed.

So it is clear that some of the ETH may be returned from the operator to the OperatorProposal contract.

The issue is that the remaining ETH is not refunded to the executor and therefore this results in a direct loss of funds for the executor.

I discussed this issue with the sponsor and it is clear that the remaining ETH needs to be refunded when allowOperatorsToSpendPartyEth=false.

However it is not clear what to do when allowOperatorsToSpendPartyEth=true. In this case ETH can be spent from the party's balance. So there should be limited use cases for the executor providing additional ETH.

But if the executor provides additional ETH what should happen?
Should the ETH be taken from the executor first? Or should it be taken from the Party balance first?

The sponsor mentioned that since there are limited use cases for the executor providing additional ETH it may be ok to not refund ETH at all.

I disagree with this. Even when allowOperatorsToSpendPartyEth=true there should be a policy for refunds. I.e. the necessary ETH should either be taken from the Party's balance or from the executor first and any remaining funds from the executor should be returned.
However since it is not clear how to proceed in this case and since it is less important compared to the case where allowOperatorsToSpendPartyEth=false I will only make a suggestion for the case where allowOperatorsToSpendPartyEth=false.

The sponsor should decide what to do in the other case and make the appropriate changes.

Proof of Concept

When the executor executes an OperatorProposal, operatorValue amount of ETH is sent to the operator contract (when allowOperatorsToSpendPartyEth=false all of these funds must come from the msg.value):

Link

if (!allowOperatorsToSpendPartyEth && data.operatorValue > msg.value) {
    revert NotEnoughEthError(data.operatorValue, msg.value);
}


// Execute the operation.
data.operator.execute{ value: data.operatorValue }(data.operatorData, executionData);

Currently the only operator contract that is implemented is the CollectionBatchBuyOperator and as explained above not all of the funds may be used so the funds are sent back to the Party:

Link

uint256 unusedEth = msg.value - totalEthUsed;
if (unusedEth > 0) payable(msg.sender).transferEth(unusedEth);

However after calling the operator contract, the OperatorProposal contract just returns without sending back the unused funds to the executor (msg.sender).

Link

// Nothing left to do.
return "";

So there is a loss of funds for the executor. The leftover funds are effectively transferred to the Party.

Tools Used

VSCode

Recommended Mitigation Steps

As mentioned before, this is only a fix for the case when allowOperatorsToSpendPartyEth=false.

Fix:

diff --git a/contracts/proposals/OperatorProposal.sol b/contracts/proposals/OperatorProposal.sol
index 23e2897..507e0d5 100644
--- a/contracts/proposals/OperatorProposal.sol
+++ b/contracts/proposals/OperatorProposal.sol
@@ -4,7 +4,11 @@ pragma solidity 0.8.17;
 import "./IProposalExecutionEngine.sol";
 import "../operators/IOperator.sol";
 
+import "../utils/LibAddress.sol";
+
 contract OperatorProposal {
+    using LibAddress for address payable;
+    
     struct OperatorProposalData {
         // Addresses that are allowed to execute the proposal and decide what
         // calldata used by the operator proposal at the time of execution.
@@ -41,9 +45,17 @@ contract OperatorProposal {
             revert NotEnoughEthError(data.operatorValue, msg.value);
         }
 
+        uint256 partyBalanceBefore = address(this).balance - msg.value;
+
         // Execute the operation.
         data.operator.execute{ value: data.operatorValue }(data.operatorData, executionData);
 
+        if (!allowOperatorsToSpendPartyEth) {
+            if (address(this).balance - partyBalanceBefore > 0) {
+                payable(msg.sender).transferEth(address(this).balance - partyBalanceBefore);
+            }
+        }
+
         // Nothing left to do.
         return "";
     }

[c4-judge]

0xean marked the issue as duplicate of #30

[c4-judge]

0xean marked the issue as not a duplicate

[c4-judge]

0xean marked the issue as primary issue

[0xean]

Marking this as primary issue since it has a bit more context. Looking forward to sponsor comment on this. As described in my comment on #30 I am not entirely sure that this qualifies as M based on the fact that the caller presumably is able to call with the correct value. QA may be more appropriate

[HollaDieWaldfee100]

@0xean
It is not true that the caller can just use the function with the correct value:

For example the CollectionBatchBuyOperator contract may not spend all of the ETH because the actual purchases that are made are not necessarily known at the time the proposal is created. Also not all purchases may succeed.

[0xble]

@0xean It is not true that the caller can just use the function with the correct value:

For example the CollectionBatchBuyOperator contract may not spend all of the ETH because the actual purchases that are made are not necessarily known at the time the proposal is created. Also not all purchases may succeed.

@HollaDieWaldfee100 is right, there is at least one case with the CollectionBatchBuyOperator where it may not use all the ETH and the user may expect to be refunded. I think marking as M is valid.

[c4-sponsor]

0xble marked the issue as sponsor confirmed

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[EvanYu0816]

@0xean Sorry I was in a time crunch this contest and I didn't get a chance to finish my QA report... but this is what I meant for my low 2. Is there a chance I can get a partial-25 here? If not, I totally understand.

https://github.com/code-423n4/2023-04-party-findings/blob/main/data/evan-Q.md#low-2-collectionbatchbuyoperator-doesnt-refund-operator-value-if-operator-spent-their-own-fund

[0xean]

@0xean Sorry I was in a time crunch this contest and I didn't get a chance to finish my QA report... but this is what I meant for my low 2. Is there a chance I can get a partial-25 here? If not, I totally understand.

https://github.com/code-423n4/2023-04-party-findings/blob/main/data/evan-Q.md#low-2-collectionbatchbuyoperator-doesnt-refund-operator-value-if-operator-spent-their-own-fund

No, I am sorry, the contests are time based for a reason and your report isn't satisfactory on its own.