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

fix: contributor fee mitigations #301

Merged
merged 10 commits into from
Sep 18, 2023

Conversation

arr00
Copy link
Contributor

@arr00 arr00 commented Sep 18, 2023

No description provided.

@height
Copy link

height bot commented Sep 18, 2023

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

Codecov Report

Patch coverage: 64.00% and project coverage change: +0.01% 🎉

Comparison is base (9b5f379) 60.54% compared to head (f7c5190) 60.55%.

Additional details and impacted files
@@                    Coverage Diff                    @@
##           feat/contribution-fee     #301      +/-   ##
=========================================================
+ Coverage                  60.54%   60.55%   +0.01%     
=========================================================
  Files                         63       63              
  Lines                       2471     2477       +6     
  Branches                     574      574              
=========================================================
+ Hits                        1496     1500       +4     
- Misses                       778      782       +4     
+ Partials                     197      195       -2     
Files Changed Coverage Δ
contracts/crowdfund/Crowdfund.sol 52.57% <0.00%> (-0.92%) ⬇️
contracts/crowdfund/ETHCrowdfundBase.sol 80.37% <ø> (ø)
contracts/gatekeepers/AllowListGateKeeper.sol 77.77% <0.00%> (-22.23%) ⬇️
contracts/gatekeepers/TokenGateKeeper.sol 88.88% <50.00%> (-11.12%) ⬇️
contracts/crowdfund/ContributionRouter.sol 100.00% <100.00%> (ø)
contracts/crowdfund/InitialETHCrowdfund.sol 86.56% <100.00%> (+4.87%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@0xble 0xble left a comment

Choose a reason for hiding this comment

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

Some notes, otherwise looks good. It appears to address each of the following:

  • Fix excess ETH is lost when maxTotalContributions issue
    • We’re just going to revert when excess ETH is returned (in receiver())
  • Fix batchContributeFor calling itself
  • Fix gateKeeper checks relying on msg.sender

And looks like we are ignoring the following suggestions, which is fine:

  • Add timelock for owner changing feePerMint
  • Allow user making contribution to himself through ContributionRouter to set delegate

contracts/crowdfund/ContributionRouter.sol Outdated Show resolved Hide resolved
function batchContributeFor(
address[] memory recipients,
address[] memory initialDelegates,
uint256[] memory values,
bytes[] memory gateDatas,
bool revertOnFailure
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it does, but note to self that we'll have to check with FE this doesn't break anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good callout. Agree that it's probably fine.

@arr00 arr00 marked this pull request as ready for review September 18, 2023 21:10
@arr00 arr00 merged commit 155b275 into feat/contribution-fee Sep 18, 2023
3 checks passed
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.

3 participants