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.

Sign up for GitHub

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

Code review via comments for YieldDistributor.sol #65

Closed
wants to merge 1 commit into from
Closed

Code review via comments for YieldDistributor.sol #65

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions src/YieldDistributor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ contract YieldDistributor is OwnableUpgradeable {
address[] public queuedProjectsForRemoval;
// @notice The voting power allocated to projects by voters in the current cycle
uint256[] public projectDistributions;
/* IIUC, `projectDistributions` and `projects` are intended to be kept in sync.
- This is a somewhat fragile pattern. At the very least, it should be commented and made explicit.
- It would be better to have a single array with a struct holding the address and uint256. That is
not gas-optimal, but given that this contract lives on Gnosis I think the balance between code
quality and gas optimization should tilt heavily towards the former.
- Even better IMO would be a `mapping(address => uint256)` to track distributions per project.
That would allow a voting interface that passes votes by address rather than array index, which
is pretty hard to debug after the fact (e.g. through a chain explorer). The current setup is more
vulnerable to either of an evil frontend or MEV. Neither of those is a proximate issue, but still.
Copy link
Author

Choose a reason for hiding this comment

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

It occurs to me that more probable than an evil frontend is a bug in the real frontend that causes the displayed project order to mismatch with the contract project order. Protecting against that seems worthwhile.

*/
// @notice The last block number in which a specified account cast a vote
mapping(address => uint256) public accountLastVoted;

Expand Down Expand Up @@ -121,6 +131,14 @@ contract YieldDistributor is OwnableUpgradeable {
* @return uint256 The voting power of the user
*/
function getCurrentVotingPower(address _account) public view returns (uint256) {
/*
- This returns the voting power based on the last period, which is intended but may be
surprising. It would be good to comment this behavior.
Comment on lines +135 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

Voting power at the time of distribution required us to iterate over all the voters, introducing an attack vector. This prompted us to record voting power at the time of voting and not distribution. This would, however, introduce an advantage to "late voters", so we opted to calculate it at a set delay.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, just think it deserves a comment.

- It also uses `cycleLength` (the minimum time between cycles) for this calculation rather
than the actual time for the block before the last claimed block. If this is an intentional
choice it should be explicitly called out, but I think it might make more sense to track the
last two distribution blocks, rather than just `lastClaimedBlockNumber` here.
Comment on lines +139 to +140
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand, the suggestion is to define an interval as a tuple of two blocks, rather than using lastClaimedBlockNumber? What are the benefits in this approach?

Copy link
Author

Choose a reason for hiding this comment

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

The only benefit is that it will measure the power accumulated over the previous period between distributions, rather than the cycleLength before the previous distribution. I believe that in normal operation, these are expected to be very close. There is only a difference when distributions are not occurring on a cycleLength candence.

Consider the following scenario:

cycleLength == 100
Block 100: distribution
Block 200: no distribution (because the mechanism is broken or for some other reason)
Block 300: distribution
Block 400: distribution

IIUC, the distribution in block 400 will use the voting power accumulated between blocks 200 and 300. I'm proposing that it may be more fair to use the voting power accumulated between blocks 100 and 300 in this scenario.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bagelface thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ShaNagbaImuru on this. cycleLength would be more accurately considered the "minimum cycle length" before which a distribution can't happen. Assuming the suggested changes don't have any major tradeoffs, I think it makes for a better algorithm.

*/
return this.getVotingPowerForPeriod(lastClaimedBlockNumber - cycleLength, lastClaimedBlockNumber, _account);
}

Expand Down Expand Up @@ -170,6 +188,9 @@ contract YieldDistributor is OwnableUpgradeable {

_latestKey = _currentCheckpoint._key;
}
/* It really feels like L170-190 could be unified into a single loop rather than a separate
step for calculating just the first chunk of voting power.
*/
Comment on lines +191 to +193
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion re: this

function getVotingPowerForPeriod(uint256 _start, uint256 _end, address _account) external view returns (uint256) {
    if (_start >= _end) revert StartMustBeBeforeEnd();
    if (_end > block.number) revert EndAfterCurrentBlock();

    uint32 checkpointCount = BREAD.numCheckpoints(_account);
    if (checkpointCount == 0) return 0;

    Checkpoints.Checkpoint208 memory checkpoint = BREAD.checkpoints(_account, 0);
    if (checkpoint._key > _end) return 0;

    uint256 totalVotingPower = 0;
    for (uint32 i = checkpointCount; i > 0;) {
        checkpoint = BREAD.checkpoints(_account, --i);

        if (checkpoint._key <= _end) {
            uint48 effectiveStart = checkpoint._key < _start ? uint48(_start) : checkpoint._key;
            totalVotingPower += checkpoint._value * (_end - effectiveStart);

            if (checkpoint._key <= _start) {
                totalVotingPower -= checkpoint._value * (_start - checkpoint._key);
                break;
            }
            _end = checkpoint._key;
        }
    }

    return totalVotingPower;
}

Thoughts @ShaNagbaImuru ?

Copy link
Author

Choose a reason for hiding this comment

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

  1. It might be cleaner to use a continue when the checkpoint._key > end, to reduce the indent required.
  2. I don't think I understand why you need both of calculating and using effectiveStart and subtracting voting power when checkpoint._key <= _start. That seems redundant unless I'm missing something. (If it is redundant, I prefer the first approach for clarity.)
  3. I think there's probably already unit testing for this code, but if not there should be, to ensure that this change in logic doesn't change the results.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, opened #84 for this, will get back in touch regarding this


return _totalVotingPower;
}
Expand Down Expand Up @@ -202,7 +223,9 @@ contract YieldDistributor is OwnableUpgradeable {

BREAD.claimYield(BREAD.yieldAccrued(), address(this));
lastClaimedBlockNumber = block.number;

/* This is presumably leaving some dust in this contract, which is probably fine but may
deserve a callout comment.
*/
ShaNagbaImuru marked this conversation as resolved.
Show resolved Hide resolved
uint256 _halfYield = BREAD.balanceOf(address(this)) / 2;
uint256 _baseSplit = _halfYield / projects.length;

Expand Down Expand Up @@ -236,6 +259,8 @@ contract YieldDistributor is OwnableUpgradeable {
/**
* @notice Internal function for casting votes for a specified user
* @param _account Address of user to cast votes for
* /* Unless I'm missing something, calling these basis points is pretty confusing, since that term
* usually refers to 1/100th of a percent, which these are not.
Copy link
Collaborator

Choose a reason for hiding this comment

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

these points can be up to 10000, does that make sense?

Copy link
Author

Choose a reason for hiding this comment

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

That's param-controlled and changeable, right? So they happen to be basis points currently but that is contingent. That does explain it though.

* @param _points Basis points for calculating the amount of votes cast
* @param _votingPower Amount of voting power being cast
*/
Expand Down Expand Up @@ -279,6 +304,8 @@ contract YieldDistributor is OwnableUpgradeable {
for (uint256 i; i < _oldProjects.length; ++i) {
address _project = _oldProjects[i];
bool _remove;
/* Using a mapping would eliminate the need for this inner loop.
*/

for (uint256 j; j < queuedProjectsForRemoval.length; ++j) {
if (_project == queuedProjectsForRemoval[j]) {
Expand Down Expand Up @@ -307,7 +334,11 @@ contract YieldDistributor is OwnableUpgradeable {
revert AlreadyMemberProject();
}
}

/* Arguably this method should check if the project is in the removal queue and remove it from
there if so. As is, if someone fat-fingers their way into removing a project, there's no way to
undo that, since a project that is in both the addition and removal queues will end up removed
no matter what.
*/
for (uint256 i; i < queuedProjectsForAddition.length; ++i) {
if (queuedProjectsForAddition[i] == _project) {
revert ProjectAlreadyQueued();
Expand All @@ -329,6 +360,9 @@ contract YieldDistributor is OwnableUpgradeable {
}
}

/* Just a callout that if this method is altered to check the addition queue, it'll have to
change this check as well.
*/
if (!_found) revert ProjectNotFound();

for (uint256 i; i < queuedProjectsForRemoval.length; ++i) {
Expand Down