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

Canceling proposal breaks party #343

Closed
code423n4 opened this issue Sep 19, 2022 · 2 comments
Closed

Canceling proposal breaks party #343

code423n4 opened this issue Sep 19, 2022 · 2 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/party/PartyGovernance.sol#L712

Vulnerability details

Impact

can leave a party broken locking all or any assets

Recommended Mitigation Steps

Trasnfer any/all funds to another party contract escentially setup a new party before taking the last resort of canceling a proposal that could potentially leave a party in a broken state, by putting this in place, if some how is less experienced carries out this activity there is at least some portection in place, as it is stated this should be a last resort only, propper precautions should be put into place to protect any parties that may need to do this.

it does not seem to be good practice to allow a function that can potentially break a party without having some way of protecting any assets that may be held, it could most certainly loose trust in the project by its users.

/// @dev proposal.cancelDelay seconds must have passed since it was first
/// executed for this to be valid.
/// The currently active proposal will simply be yeeted out of existence
/// so another proposal can execute.
/// This is intended to be a last resort and can leave the party
/// in a broken state. Whenever possible, active proposals should be
/// allowed to complete their lifecycle

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 19, 2022
code423n4 added a commit that referenced this issue Sep 19, 2022
@merklejerk merklejerk added the sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Sep 22, 2022
@merklejerk
Copy link
Collaborator

merklejerk commented Sep 22, 2022

This is valid criticism but also a known limitation we called out earlier and have accepted for this iteration. As mentioned, cancels are a last resort.

@HardlyDifficult
Copy link
Collaborator

This report is just restating the risk from comments, but the rec is a potential improvement suggestion. Merging with #347

@HardlyDifficult HardlyDifficult added duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants