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

[delta-audit] bonding: Various fixes from audit findings #622

Merged
merged 8 commits into from
Sep 26, 2023

Conversation

victorges
Copy link
Member

@victorges victorges commented Sep 7, 2023

What does this pull request do? Explain your changes. (required)
This mitigates a couple minor findings from the audit that either haven't been flagged by wardens but we found through self-review (e.g. duplicate checkpoints) or too minor issue to have its own PR (solidity version). Even though I'm combining all the fixes in this PR, each mitigation is done on a separate commit to facilitate review.

Specific updates (required)

  • Avoid double-checkpointing of transcoders when making self-bonding actions
  • Add the previous/newLastClaimRound to the DelegatorBondedAmountChanged event (otherwise some changes would be silent on the event side, like unbonding exactly the amount of pending rewards)

The minorest:

  • Fix solidity version in IBondingVotes.sol file
  • Add a remappings.txt file to the project, which improves VS Code suggestions and navigation
  • Fix a few misleading or typod documentations

How did you test each of these updates (required)
Added tests to all the functional changes and yarn test

Will deploy on devnet after merged.

Does this pull request close any open issues?
N/A

Checklist:

  • README and other documentation updated
  • All tests using yarn test pass

@victorges victorges force-pushed the vg/fix/audit branch 2 times, most recently from 429b877 to 427a77e Compare September 20, 2023 21:07
@victorges victorges changed the title [WIP] Delta audit fixes [delta-audit] Various fixes from audit findings Sep 20, 2023
@victorges victorges changed the title [delta-audit] Various fixes from audit findings [delta-audit] bonding: Various fixes from audit findings Sep 20, 2023
@victorges victorges marked this pull request as ready for review September 20, 2023 21:35
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Just a few minor comments.

contracts/bonding/BondingManager.sol Outdated Show resolved Hide resolved
contracts/bonding/BondingManager.sol Outdated Show resolved Hide resolved
We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM

@victorges victorges merged commit e18ed98 into delta Sep 26, 2023
3 checks passed
@victorges victorges deleted the vg/fix/audit branch September 26, 2023 00:43
victorges added a commit that referenced this pull request Sep 27, 2023
* Add remappings file for vs code

* bonding: Avoid double-checkpoint on self-bond/unbond

* bonding: Fix solidity version for IBondingVotes

* bonding: Add lastClaimRound to delegator change event

* bonding,treasury: Fix a few documentations

QA Report code-423n4/2023-08-livepeer-findings#150

* bonding: Improve docs about unused _endRound

Reported on: code-423n4/2023-08-livepeer-findings#205

* bonding: Address PR comments

* bonding: Add deprecation notice to slashTranscoder

We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
victorges added a commit that referenced this pull request Oct 11, 2023
* Add remappings file for vs code

* bonding: Avoid double-checkpoint on self-bond/unbond

* bonding: Fix solidity version for IBondingVotes

* bonding: Add lastClaimRound to delegator change event

* bonding,treasury: Fix a few documentations

QA Report code-423n4/2023-08-livepeer-findings#150

* bonding: Improve docs about unused _endRound

Reported on: code-423n4/2023-08-livepeer-findings#205

* bonding: Address PR comments

* bonding: Add deprecation notice to slashTranscoder

We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
victorges added a commit that referenced this pull request Oct 11, 2023
* Add remappings file for vs code

* bonding: Avoid double-checkpoint on self-bond/unbond

* bonding: Fix solidity version for IBondingVotes

* bonding: Add lastClaimRound to delegator change event

* bonding,treasury: Fix a few documentations

QA Report code-423n4/2023-08-livepeer-findings#150

* bonding: Improve docs about unused _endRound

Reported on: code-423n4/2023-08-livepeer-findings#205

* bonding: Address PR comments

* bonding: Add deprecation notice to slashTranscoder

We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
victorges added a commit that referenced this pull request Oct 11, 2023
* Add remappings file for vs code

* bonding: Avoid double-checkpoint on self-bond/unbond

* bonding: Fix solidity version for IBondingVotes

* bonding: Add lastClaimRound to delegator change event

* bonding,treasury: Fix a few documentations

QA Report code-423n4/2023-08-livepeer-findings#150

* bonding: Improve docs about unused _endRound

Reported on: code-423n4/2023-08-livepeer-findings#205

* bonding: Address PR comments

* bonding: Add deprecation notice to slashTranscoder

We've had some many wardens spending their time auditing this
function and the side-effects of it, when it is disabled and
so doesn't really matter. Adding the comment so we don't need
to remember on the next audit to make this disclaimer.
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