Lines of code

https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L789 https://github.com/code-423n4/2022-08-nounsdao/blob/main/contracts/governance/NounsDAOLogicV2.sol#L983

Vulnerability details

Impact

It's considered a best practice to always check the return of the transaction when sending Ether with .call, since it's possible for a tx failure due to external factors out of the contract control.

Currently, the contract emits an event with the result but it doesn't throw in case of failure.

I would recommend to ensure the transaction throws in case of failure instead of emitting an event with the boolean result of false.

Proof of Concept

There's two functions sending ether: _withdraw() and _refundGas().

For the case of the external function withdraw(), failing to send Ether will be a successful transaction with the Withdraw event having the sent value as false.

And in the case of refundGas(), the function is called with this flow: castRefundableVote() -> castRefundableVoteInternal() -> _refundGas().

Even if the function fails to send ether (refund the gas)storage variables will still be updated (the proposal and receipt inside castVoteInternal() called by castRefundableVoteInternal()) and, similarly to withdraw(), it will be a succesful transaction with an RefundableVote having the refundSent as false.

Tools used

Manual review

Recommended Mitigation Steps

Check the return of .call inside _withdraw() and inside _refundGas().

$ git diff --no-index NounsDAOLogicV2.sol.orig NounsDAOLogicV2.sol
diff --git a/NounsDAOLogicV2.sol.orig b/NounsDAOLogicV2.sol
index f02b5c2..a03554d 100644
--- a/NounsDAOLogicV2.sol.orig
+++ b/NounsDAOLogicV2.sol
@@ -787,8 +787,11 @@ contract NounsDAOLogicV2 is NounsDAOStorageV2, NounsDAOEventsV2 {

         uint256 amount = address(this).balance;
         (bool sent, ) = msg.sender.call{ value: amount }('');
-
-        emit Withdraw(amount, sent);
+        if (!sent) {
+            // Add a new custom error for a failed transaction.
+            revert ErrorWithdraw();
+        }
+        emit Withdraw(amount);
     }

@@ -981,7 +984,11 @@ contract NounsDAOLogicV2 is NounsDAOStorageV2, NounsDAOEventsV2 {
             uint256 gasUsed = startGas - gasleft() + REFUND_BASE_GAS;
             uint256 refundAmount = min(gasPrice * gasUsed, balance);
             (bool refundSent, ) = msg.sender.call{ value: refundAmount }('');
-            emit RefundableVote(msg.sender, refundAmount, refundSent);
+            if (!refundSent) {
+                // Add a new custom error for a failed transaction.
+                revert ErrorRefundableVote();
+            }
+            emit RefundableVote(msg.sender, refundAmount);
         }
     }

Alternatively, for _refundGas(), if the idea is to update the storage variables even if no gas is refunded, the contract could save the gas transaction variables into the state and add a separate function used exclusively to refund the gas (decoupled from castRefundableVoteInternal()).

code-423n4 / 2022-08-nounsdao-findings