Skip to content
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

OOG invalidates proposal #12

Merged
merged 10 commits into from
Mar 9, 2021
Merged

OOG invalidates proposal #12

merged 10 commits into from
Mar 9, 2021

Conversation

rmeissner
Copy link
Contributor

@rmeissner rmeissner commented Mar 7, 2021

This PR will require the module transaction to be successfull and add the possibility to set an expiration time for the answer.

For more details on why this is done see below.

Problem

A malicious executor could send to little gas that would then mark that particular transaction as executed even so internally it failed because there was not enough gas available. This potentially prevents execution of proposals.

See https://github.com/gnosis/dao-module/blob/main/contracts/DaoModule.sol#L220

Solutions

Tx gas

Add a "gas" value to the transactions, that set a minimum of required gas for each transactions. If the transaction fails (no matter why) it will still me marked as executed. Following transactions in the array will be able to be executed, but it will not possible to retry the failed transaction.

Never fail

Require that the module transaction did not fail. This will prevent the execution of other tx in the array and the tx will not be marked as executed. Unless the proposal is invalidated it is possible to retry the transaction (at any point in time).
-> This bring up the question if a tx/ proposal should have a deadline for execution.

test/DaoModule.spec.ts Outdated Show resolved Hide resolved
@rmeissner rmeissner requested a review from fedgiac March 8, 2021 20:53
Copy link
Collaborator

@auryn-macmillan auryn-macmillan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Copy link
Contributor

@fedgiac fedgiac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@rmeissner rmeissner merged commit 7a5244d into main Mar 9, 2021
@rmeissner rmeissner deleted the feature/dos_oog branch March 9, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants