audit: prevent reentrancy issues in execute function (#207)

* audit: prevent reentrancy issues in execute function * comment: add comment for cached proposal * comment: remove comment for finalization status modification * chore: CEI explanation * chore: typo --------- Co-authored-by: Orlando <[email protected]>
snapshot-labs · Jun 19, 2023 · e7c71b2 · e7c71b2
1 parent 2c5b49f
commit e7c71b2
Showing 1 changed file with 8 additions and 6 deletions.
diff --git a/src/Space.sol b/src/Space.sol
@@ -278,19 +278,21 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
         Proposal storage proposal = proposals[proposalId];
         _assertProposalExists(proposal);
 
-        // We add reentrancy protection here to prevent this function being re-entered by the execution strategy.
-        // We cannot use the Checks-Effects-Interactions pattern because the proposal status is checked inside
-        // the execution strategy (so essentially forced to do Checks-Interactions-Effects).
+        // We cache the proposal because we will modify the *real* proposal's finalizationStatus before
+        // calling the `execute` function. We will use the `cachedProposal` as an argument to the `execute` function.
+        // This allows us to follow the CEI pattern to avoid reentrancy issues.
+        Proposal memory cachedProposal = proposal;
+
+        proposal.finalizationStatus = FinalizationStatus.Executed;
+
         proposal.executionStrategy.execute(
-            proposal,
+            cachedProposal,
             votePower[proposalId][Choice.For],
             votePower[proposalId][Choice.Against],
             votePower[proposalId][Choice.Abstain],
             executionPayload
         );
 
-        proposal.finalizationStatus = FinalizationStatus.Executed;
-
         emit ProposalExecuted(proposalId);
     }