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

Bug Impossible remove proposal that flow don't exist + improvments #8

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kamikazebr
Copy link
Member

✨ solidity 0.8.27 last version
⚡ require custom error
⚡ fix tests for fork
⚡ internal storages all publics
⚡ new events and errors
🐛 try catchs
⚡ fix setup script

⚡ require custom error
⚡ fix tests for fork
⚡ internal storages all publics
⚡ new events and errors
🐛  try catchs
⚡ fix setup script
Copy link

@Corantin Corantin left a comment

Choose a reason for hiding this comment

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

LGTM, so the try catch is allowing others to sync ?
Do you know if 171 going to be up gain when enough support ?
That might be a edge case we never had before. (0 -> enough support -> not enough -> enough)

@kamikazebr
Copy link
Member Author

LGTM, so the try catch is allowing others to sync ? Do you know if 171 going to be up gain when enough support ? That might be a edge case we never had before. (0 -> enough support -> not enough -> enough)

Yeah try catch make sure others proposals dont get blocked by one with error.

171 dont exist, so need be created new proposal for it, but if its possible to back in GV1, just need be registered, activated again and will work without problem.

Not, its not a edge case, that was a bug, 171 was should be deleted but because the error never got deleted and create a inconsistent state, DoSing the contracts. High vuln

Comment on lines -45 to +50
mapping(uint256 => Flow) internal flows;
mapping(uint256 => Proposal) internal registeredProposals;
mapping(address => bool) internal registeredBeneficiary;
uint256[15] internal activeProposals;
mapping(uint256 => Flow) public flows;
mapping(uint256 => Proposal) public registeredProposals;
mapping(address => bool) public registeredBeneficiary;
uint256[15] public activeProposals;
Copy link
Member Author

Choose a reason for hiding this comment

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

made all public to make easy to investigate the state of the contracts.

Comment on lines +58 to +59
event FlowUpdatedError(uint256 proposalId, address beneficiary, uint256 rate, bytes error);
event DeleteFlowFailed(uint256 proposalId, bytes error);
Copy link
Member Author

Choose a reason for hiding this comment

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

bytes error hold the original error

function removeProposals(uint256[] memory _proposalIds) public onlyOwner {
for (uint256 i = 0; i < _proposalIds.length; i++) {
_removeProposal(_proposalIds[i]);
function removeProposals(uint256[] memory _proposalIndexes) public onlyOwner {
Copy link
Member Author

Choose a reason for hiding this comment

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

right name, that ints indexes and not ids

Comment on lines +171 to +173
require(_proposalId != 0, InvalidProposalId());
require(_beneficiary != address(0), InvalidBeneficiaryAddress());
require(!registeredBeneficiary[_beneficiary], BeneficiaryAlreadyRegistered());
Copy link
Member Author

Choose a reason for hiding this comment

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

the beauty

} catch (bytes memory error) {
// console.log("Flow update failed:");
// console.logBytes(error);
emit FlowUpdatedError(_proposalId, registeredProposals[_proposalId].beneficiary, flow.lastRate, error);
Copy link
Member Author

Choose a reason for hiding this comment

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

avoid revert and DoS the contract

// console.log("Proposal id %d", proposalId);
try superfluid.deleteFlow(token, registeredProposals[proposalId].beneficiary){
// console.log("Flow deleted");
}catch(bytes memory error){
Copy link
Member Author

Choose a reason for hiding this comment

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

Flow not created error can happens by Superfluid side that can revert here without try catch and DoS the contract

Comment on lines +17 to +18
uint256 WRAP_AMOUNT = 100 ether;
uint256 CEILING_BSP = 250; // 2.5% of Common Pool expresed as Basis Points
Copy link
Member Author

Choose a reason for hiding this comment

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

matching the current values

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.

2 participants