New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(og): N-02 - Remove payable in executeProposal #4491
Conversation
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -297,7 +297,7 @@ contract OptimisticGovernor is OptimisticOracleV3CallbackRecipientInterface, Mod | |||
* @notice Executes an approved proposal. | |||
* @param _transactions the transactions being executed. These must exactly match those that were proposed. | |||
*/ | |||
function executeProposal(Transaction[] memory _transactions) external payable nonReentrant { | |||
function executeProposal(Transaction[] memory _transactions) external nonReentrant { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would the value passed in not be pulled out on exec
with transaction.value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, exec
does not pass msg.value
when calling execTransactionFromModule
on the target (safe) here. The value
parameter in execTransactionFromModule
would pull Ether from the safe in case of regular call operation, but any value sent by the caller of executeProposal
would be stuck in the Optimistic Governor contract.
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. thanks for explaining. I was a bit confused by the exec
function.
Motivation
OZ identified the following issue:
Summary
This PR addresses this issue by removing the payable attribute to avoid potentially locking ETH in the OptimisticGovernor contract.
Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.