From e7c71b2d00f30b8f4d18984c3c70eafbf1b9f2ae Mon Sep 17 00:00:00 2001 From: pscott <30843220+pscott@users.noreply.github.com> Date: Mon, 19 Jun 2023 13:24:10 +0200 Subject: [PATCH] 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 --- src/Space.sol | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Space.sol b/src/Space.sol index 9ebef6b8..27ae9cc9 100644 --- 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); }