yAudit veYFI Review

The purpose of veYFI is to transition the YFI token to the vote escrow (ve) tokenomics model. The Yearn multisig will manage the contracts, which can upgrade and migrate as needed. The GaugeFactory contract creates new Gauge and ExtraReward contracts for each Yearn Vault. The Registry contract maintains a list of active gauges and rewards pools with their corresponding vaults, which enables upgrades to the system. The Gauge contract handles deposits of YFI while the VotingEscrow.vy contract, forked and modified from Curve's ve tokenomics design, handles deposits of veYFI to enable governance rights. Some differences from Curve's ve tokenomics include the ability to withdraw locked veYFI with a penalty and the ability for locked veYFI holders to delegate votes.

The main branch of the veYFI Repo was reviewed over 20 days. The code review was performed by 2 auditors between March 28 and April 16, 2022. The repository was under active development during the review, but the review was limited to one specific commit.

Scope

Code Repo Commit

The commit reviewed was 7846291efec36638eae32ac8798544050ca20877. The review covered the entire repository at this specific commit but focused on the contracts directory.

After the findings were presented to the veYFI team, fixes were made and included in commit bbecf1e2ceaac9020658d34755ee769b57d54374. Fixes were reviewed to confirm whether they remedied the original finding, but a new security review was not performed on the revised code (e.g. to determine whether new vulnerabilities were introduced by the fixes). Reviewers note that VeYFIRewards.sol was replaced by VeYFIRewards.vy between commits, and the two files appear to have significant differences in the code.

The review is a code review to identify potential vulnerabilities in the code. The reviewers did not investigate security practices or operational security and assumed that privileged accounts could be trusted. The reviewers did not evaluate the security of the code relative to a standard or specification. The review may not have identified all potential attack vectors or areas of vulnerability.

yAudit and the auditors make no warranties regarding the security of the code and do not warrant that the code is free from defects. yAudit and the auditors do not represent nor imply to third party users that the code has been audited nor that the code is free from defects. By deploying or using the code, Yearn and users agree to use the code at their own risk.

Code Evaluation Matrix

Category	Mark	Description
Access Control	Good	The onlyOwner modifier was applied on functions that set key protocol state variables in Registry.sol and BaseGauge.sol. Elsewhere, msg.sender is used so that the user can only control their own deposits. Access controls are applied where needed.
Mathematics	Average	Solidity 0.8.13 is used, which provides overflow and underflow protect. No unchecked code exists and no low-level bitwise operations are performed. There was no unusually complex math, except perhaps some of the vyper code functions borrowed from Curve which were not modified.
Complexity	Low	The inheritance structure, duplicate function names, lack of clear comments, and inclusion of vyper code all together can make the code hard to follow at times. There is a lack of clarity around the off-chain voting process, and no comments in the code or repository issues/PRs clarify how the off-chain voting expected to integrate with on-chain voting variables.
Libraries	Good	Only basic Open Zeppelin contracts such as SafeERC20, Ownable, and Math are imported, no other external libraries are used. Fewer and simpler external dependencies is always a plus for security.
Decentralization	Average	The onlyOwner modifier indicates there is some centralization risk, but if the owner is a Yearn Finance multisig, the risk is reduced. Contracts such as VotingEscrow.vy support migration, so some form of ownership is required.
Code stability	Average	Changes were reviewed at a specific commit and the scope was not expanded after the review was started. However, development was ongoing when the review was performed so the code was not fully frozen, which means deployed code may vary from what was reviewed.
Documentation	Low	Comments existed in many places, but were lacking in key areas. As one example, identically named _updateReward functions existed in Gauge.sol, ExtraReward.sol and VeYfiRewards.sol, but no comments existed on either function and no explanation of the differences of these identically-named function existed. It would be best if more thorough comments and documentation was added throughout the code to better explain the purpose of different functions and specific math that is performed. No developer documentation like gitbooks was observed for veYFI at the time of review.
Monitoring	Good	Events were added to all important functions that modified state variables.
Testing and verification	Average	Brownie test coverage was passing with some warnings at the commit reviewed. Test coverage was above 80% in most contract functions but not all. There was a lack of testing integration with snapshot.org voting contracts and off-chain voting strategies.

Findings Explanation

Findings are broken down into sections by their respective impact:

Critical, High, Medium, Low impact
- These are findings that range from attacks that may cause loss of funds, impact control/ownership of the contracts, or cause any unintended consequences/actions that are outside the scope of the requirements
Gas savings
- Findings that can improve the gas efficiency of the contracts
Informational
- Findings including recommendations and best practices

High Findings

1. High - Incompatible Vote Delegation (engn33r)

PR #99 moved veYFI to off-chain voting. Discussions with the veYFI team confirmed that the https://snapshot.org/ platform will be used for off-chain voting. The snapshot delegation documentation states the setDelegate() function should be used when a contract is delegating votes. The VoteDelegation.sol contract implements an independent vote delegation system, and there is no call in veYFI to call the setDelegate() or clearDelegate() functions in the snapshot.org delegation contract to be compatible with snapshot.org. This means the veYFI delegation code will have no impact on snapshot.org voting.

Proof of concept

The VoteDelegation.sol contract has functions including delegate() and removeDelegation(), but the VoteDelegation.sol contract does not have any interaction with the snapshot DelegateRegistry contract. Because there are no contract calls to snapshot.org contracts, snapshot.org will not register any vote delegation.

Impact

High. Vote delegation implementation is not compatible with intended offline voting approach.

Recommendation

Overall the change to off-chain voting requires additional changes and integration effort. Some of the steps required to better integrate VoteDelegation.sol with snapshot.org include:

Creating a veYFI space in snapshot.org, which requires an ENS domain as documented in: https://docs.snapshot.org/spaces/create
Adding calls to snapshot.org's existing DelegateRegistry contract function calls in several places. An example implementation from Convex is provided in the documentation: https://docs.snapshot.org/guides/delegation#with-a-smart-contract. For starters, DelegateRegistry.setDelegate() should be called in the VoteDelegation.sol _delegate() function and DelegateRegistry.clearDelegate() should be called in the _removeOldDelegation() function. Additional changes may also be needed.
Confirm that unit tests exists that interact with the DelegateRegistry contract on the proper testnet
It is not immediately clear whether the erc20-balance-of snapshot.org voting strategy works with the custom vote delegation design. This should be tested before production.

I see three different approaches to handle the off-chain voting:

Remove vote delegation to keep everything like Curve (most simple)
Remove the "until" value in the vote delegation and modify the voting delegation to use the DelegateRegistry from snapshot.org
Implement a custom voting strategy on snapshot.org to handle the delegation and "until" values (most complex)

Developer Response

The VoteDelegation.sol contract has been removed in PR #137 and it will be replaced by the suggested contract from snapshot.org.

2. High - Lock 1e-18 YFI to Get Rewards (NibblerExpress)

_updateReward() computes a penalty based on the locking ratio of the account. A maximum possible lock of the smallest possible value allows a user to avoid any penalty without locking any amount of value.

Proof of concept

def test_gauge_cheat_yfi_lock(
    yfi, ve_yfi, whale, whale_amount, shark, shark_amount, fish, fish_amount, create_vault, create_gauge, gov
):

    yfi.approve(ve_yfi, shark_amount, {"from": shark})
    ve_yfi.create_lock(
        shark_amount, chain.time() + 4 * 3600 * 24 * 365, {"from": shark}
    )
    assert yfi.balanceOf(shark) == 0

    yfi.approve(ve_yfi, fish_amount, {"from": fish})
    ve_yfi.create_lock(
        fish_amount, chain.time() + 4 * 3600 * 24 * 365, {"from": fish}
    )
    assert yfi.balanceOf(fish) == 0

    yfi.approve(ve_yfi, 1, {"from": whale})
    ve_yfi.create_lock(
        1, chain.time() + 4 * 3600 * 24 * 365, {"from": whale}
    )
    assert yfi.balanceOf(whale) == whale_amount - 1

    lp_amount = 10**18
    vault = create_vault()
    tx = create_gauge(vault)
    gauge = Gauge.at(tx.events["GaugeCreated"]["gauge"])

    vault.mint(shark, lp_amount/100)
    vault.approve(gauge, lp_amount, {"from": shark})
    gauge.deposit({"from": shark})

    vault.mint(fish, lp_amount/100)
    vault.approve(gauge, lp_amount, {"from": fish})
    gauge.deposit({"from": fish})

    vault.mint(whale, 100*lp_amount)
    vault.approve(gauge, 100*lp_amount, {"from": whale})
    gauge.deposit({"from": whale})

    yfi_to_distribute = 10**16
    yfi.mint(gov, yfi_to_distribute)
    yfi.approve(gauge, yfi_to_distribute, {"from": gov})

    gauge.queueNewRewards(yfi_to_distribute, {"from": gov})
    assert pytest.approx(gauge.rewardRate()) == yfi_to_distribute / (7 * 24 * 3600)

    chain.sleep(3600*24*7)
    chain.mine()

    assert gauge.earned(whale) < .39 * yfi_to_distribute
    gauge.getReward({"from": whale})

    assert yfi.balanceOf(whale) < .39 * yfi_to_distribute + whale_amount

    gauge.withdraw({"from": whale})
    assert vault.balanceOf(whale) == 100*lp_amount

Impact

High. Users are able to get undue rewards with minimal investment. It is not ranked critical because the user cannot flash loan their deposit to drain the contract. The user does need to keep the deposit in for seven days to reap the rewards.

Recommendation

One possible solution is to include a veYFI balance requirement when determining the penalty.

Developer Response

The veYFI balance penalty has been removed. It's replaced by a much bigger boost in PR #141. Non-boosted rewards are distributed to veYFI.

3. High - Flash Loan Sybil Attack to Boost Rewards (NibblerExpress)

The boosted balance for rewards is computed as balance * 0.4 + totalSupply * veYFIBalance / veYFITotalSypply * 0.6. (Typo in veYFITotalSupply is copied from comment in L284 of Gauge.sol.) Although depositing in your own account calls updateReward() for your account, you can use another account to flash loan a large amount of vault tokens and deposit them to boost the totalSupply. Once you have boosted totalSupply, you trigger updateReward() for the target account and then withdraw the flash loan deposit.

(Note that the inverse is also true. You can either sandwich attack someone else's update by withdrawing tokens before their update to reduce total supply, or you can withdraw tokens and use getRewardFor() to force an update to someone else's rewards like discussed here: https://github.com/yearn/veYFI/issues/33.)

Proof of concept

Boosted Balance equation: https://github.com/yAudit-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.sol#L302

Deposit Update Reward call: https://github.com/yAudit-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.sol#L351

Total Supply increase: https://github.com/yAudit-Residents/veYFI/blob/7846291efec36638eae32ac8798544050ca20877/contracts/Gauge.sol#L362

Impact

High. The magnitude of the impact depends on how much you can flash loan versus what the total supply is. If you can flash loan 5x the total supply, you can get full rewards with 1/6 the intended veYFIBalance. If you can only flash loan 10% of the total supply, you only get a 9% boost to your veYFIBalance.

Recommendation

A possible solution is to compute a time weighted average of totalSupply. Alternatively, governance could set a veYFIBalance multiplier to use instead of using totalSupply/veYFITotalSupply. A third solution is to add require(block.timestamp > lastTimeRewardApplicable()); to the _updateReward() function to prevent a user from performing multiple deposits/withdrawals in the same block.

Developer Response

Deposit and withdraw on the same block isn't possible anymore: https://github.com/yearn/veYFI/blob/bbecf1e2ceaac9020658d34755ee769b57d54374/contracts/Gauge.sol#L443-L446

It's possible to game the system with a really large amount of funds but we have added a kick function that readjusts the boost in case someone misbehaves in PR #142.

Medium Findings

1. Medium - Incorrect variables in getRewardFor call (engn33r)

The _getReward() function in Gauge.sol uses the wrong account address twice, which counts and distributes rewards incorrectly when account != msg.sender. After the review started, one of these variables was fixed, but not the other.

Proof of concept

The _account variable should be used in this line instead of msg.sender https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L512

IExtraReward(extraRewards[i]).getRewardFor(msg.sender);

The _account variable should be used in this line instead of msg.sender https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L501

IVotingEscrow(veToken).deposit_for(msg.sender, reward);

Impact

Medium. Incorrect values could be calculated when account != msg.sender in some edge cases.

Recommendation

Use this fix from PR #125 for both cases, which was created after the commit at which this code review started at.

Developer Response

Fixed

2. Medium - Unclear Integration of Vote Delegation Until Value (engn33r)

This issue is similar to the high risk voting issue but relates to a difference between Curve voting (where veYFI borrows some ideas from) and the current veYFI implementation. PR #99 moved veYFI to off-chain voting. Discussions with the veYFI team confirmed that the https://snapshot.org/ platform will be used for off-chain voting. Although snapshot.org does support vote delegation, the existing snapshot.org voting delegation does not support the "until" value. This means that if the existing snapshot.org vote delegation strategy is used (which involves adding calls to the setDelegate() and clearDelegate() functions), then delegation will remain in place until it is changed by the veYFI token owner, and can remain valid beyond the until timestamp.

Proof of concept

The VoteDelegation.sol contract has functions including delegate() and removeDelegation(), but the VoteDelegation.sol contract does not have any interaction with the snapshot DelegateRegistry contract. Because the actual act of voting is planned to happen off-chain with snapshot.org, snapshot.org needs to recognize what a valid vote or delegation is in the snapshot that it takes for each proposal. Currently there is no option in the snapshot.org delegation approach for an "until" value. Factoring in the "until" value could be implemented in a custom snapshot.org voting strategy, but because no such strategy is mentioned anywhere in the veYFI repository, it is expected that this has not been considered.

Impact

Medium. Vote delegation implementation is not compatible with intended offline voting approach. It is not considered high risk because if the snapshot.org vote delegation is integrated (which it currently is not), the basic act of vote delegation should work and a workaround with token holders calling the right function at the right time can be used.

Recommendation

The best solution is to create a custom voting strategy, instead of using the erc20-balance-of voting strategy that Curve uses. An easier solution is to remove the "until" value in the vote delegation and use the simplistic erc20-balance-of voting strategy along with the built-in snapshot.org delegation strategy.

I see three different approaches to handle the off-chain voting:

Remove vote delegation to keep everything like Curve (most simple)
Remove the "until" value in the vote delegation and modify the voting delegation to use the DelegateRegistry from snapshot.org
Implement a custom voting strategy on snapshot.org to handle the delegation and "until" values (most complex)

Developer Response

File has been removed in PR #137.

3. Medium - `force_withdraw()` penalty may inadequately deter gamification and attacks on voting (engn33r, NibblerExpress)

The penalty ratio equation reads

penalty_ratio: uint256 = min(MULTIPLIER * 3 / 4, MULTIPLIER * time_left / MAXTIME)

Short lock periods receive low withdrawal penalties, which may not be intentional. Because the MAXTIME value is 4 years, when the time_left value is less than 3 years, the penalty ratio for withdrawal will be less than 75%. This means the worst case withdrawal penalty for any lock period under 3 years is less than 75%, even in the event that the user withdraws immediately after locking. This could lead to gamification where users planning to withdraw will lock the veYFI for shorter durations instead of 4 years.

The inverse scenario could also cause a problem. Users receive a 100% credit for deposits with a four year lock but only a 75% burn. If there is a scenario where the benefit of a temporary deposit is > 75% of the deposit, a user should do a max lock and then take the burn. For example, if a proposal put forth for a vote could allow a user to drain a contract, it could be profitable for a user to deposit sufficient funds to have > 50% of the voting power and to retrieve 25% of their deposit plus what can be drained from the contract.

This attack vector would most likely happen when a user has advanced knowledge that a vote is upcoming (YIP proposals and related discussion are often public). The user could stake YFI for a specific vote (either a governance or gauge vote) where they may disproportionately benefit. This vector may be unique to veYFI compared to other ve protocols because veYFI does not apply protective measures that other protocols use. Curve does not allow for early withdrawal of locked veCRV, which protects against this scenario. Convex allows for vote delegation, similar to veYFI, but adds a 16 week cooldown period before the locked tokens can be used for voting, making this attack vector less likely due to the advanced planning necessary. https://docs.convexfinance.com/convexfinance/general-information/voting-and-gauge-weights/vote-locking

The risk for this vector is increased when combined with finding Low #3, because Low #3 outlines an approach that could enable the user paying the penalty to receive some of their penalty back in rewards.

Proof of concept

The penalty_ratio equation https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L570

Impact

Medium. The penalty calculation could be gameable because incentives are not properly aligned towards long-term locking of veYFI with the existing penalty calculations.

Recommendation

Consider modifying the function so that the penalty ratio is a function of the time_left to locked duration, not a function of the ratio of the time_left to 4 years. This could reduce gamification by increasing the penalty cost in this type of attack. The modified code below is a draft and should be thoroughly tested, but it is intended to provide results of:

If a user hypothetically locks for 4 years and withdraws in their first year, 75% penalty (same as before)
If a user hypothetically locks for 1 year and withdraws immediately, 75% penalty. With the old equation, the penalty was closer to 25%

penalty_ratio: uint256 = min(MULTIPLIER * 3 / 4, MULTIPLIER * time_left / (block.timestamp - user_point_history__ts(msg.sender, self.user_point_epoch[msg.sender])))

Require tokens to be locked for a certain time before (and possibly after) allowing voting, similar to Convex. Increasing the ease of withdrawal for locked tokens, even with a penalty, can add unexpected incentives.

Developer Response

Won't fix

Low Findings

1. Low - Voting delegation is missing edge case checks (engn33r)

The VoteDelegation.sol contract permits vote delegation. The delegation has an "until" value, which can be set to zero. The until value can be changed so that the new until value is greater than the old one, but the check should also confirm that the new until value is greater than block.timestamp. Otherwise the until value can be modified for a time in the past. This may cause issues in the voting logic depending how the until value is stored and used in the snapshot logic.

Proof of concept

The VoteDelegation.sol contract permits an until variable value of zero if the tokens are not locked. Any until variable value that is less than block.timestamp should have no impact on voting or delegation. But if the until value starts at zero, it is possible for the value to be increased to a value less than block.timestamp to reference a time in the recent past, but still have no impact. It is possible that the voting logic, which is not within the scope of these solidity contracts, may not properly consider this edge case. This edge case can be solved by adding a check in all functions that modify the until state variable, including _delegate() and increaseDelegationDuration(), to validate require(until > block.timestamp). An exception could be made for an until value of zero, resulting in a require statement that looks like require(_until > block.timestamp || _until == 0).

The amount of veYFI a user lock is based on the duration of the lock making really short locks close to negligeable. The contract distributing rewards to veYFI has been entirely changed in PR #136.

4. Low - No penalty during migration (engn33r)

The _lockingRatio() function of Gauge.sol returns a ratio based on the amount of time the user has locked their ve tokens. The maximum value for this ratio is PRECISON_FACTOR, which is when a user locks their ve tokens for MAX_TIME. A user can also have a PRECISON_FACTOR locking ratio when the ve token contract is undergoing migration and the contract owner has not call setVe() yet to point to the new ve token contract. A user can take advantage of this edge case. For example, any function with the updateReward modifier will calculate a penalty of zero when a ve token migration is ongoing, regardless of how long a user has locked their tokens.

Proof of concept

The _lockingRatio() function returns PRECISON_FACTOR, equivalent to locking for the MAX_TIME value, when the ve token is undergoing migration: https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L221

Instead, it would be better to revert in this case so that the ve token can be finished without any users taking advantage of this edge case.

Impact

Low. This is a rare edge case but should be handled properly.

Recommendation

Change the behavior of the _lockingRatio() function to revert during migration to allow the owner to call setVe() with the new ve token address.

Developer Response

Won't fix

Gas Savings Findings

1. Gas - Using "unchecked" (engn33r)

In setDuration() of BaseGauge.sol, we know periodFinish > block.timestamp so we can use the "unchecked" clause for gas savings.

The same improvement can be used in Gauge.sol.

Proof of concept

The relevant code block

if (block.timestamp < periodFinish) {
    uint256 remaining = periodFinish - block.timestamp;

Applying unchecked for gas savings, this code would look like

if (block.timestamp < periodFinish) {
    unchecked { uint256 remaining = periodFinish - block.timestamp; }

BaseGauge.sol line that can be unchecked https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L79

Gauge.sol line that can be unchecked https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L229

Impact

Gas savings

Recommendation

Use unchecked where there is no overflow or underflow risk

Developer Response

Won't fix

2. Gas - Using simple comparison (engn33r)

Using a compound comparison such as ≥ or ≤ uses more gas than a simple comparison check like >, <, or ==. Compound comparison operators can be replaced with simple ones for gas savings.

Proof of concept

The _notifyRewardAmount() function in BaseGauge.sol contains https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L170-L177

if (block.timestamp >= periodFinish) {
    rewardRate = reward / duration;
} else {
    uint256 remaining = periodFinish - block.timestamp;
    uint256 leftover = remaining * rewardRate;
    reward = reward + leftover;
    rewardRate = reward / duration;
}

By switching around the if/else clauses, we can replace the compound operator with a simple one

if (block.timestamp < periodFinish) {
    uint256 remaining = periodFinish - block.timestamp;
    uint256 leftover = remaining * rewardRate;
    reward = reward + leftover;
    rewardRate = reward / duration;
} else {
    rewardRate = reward / duration;
}

Impact

Gas savings

Recommendation

Replace compound comparison operators with simple ones for gas savings

Developer Response

Won't fix

3. Gas - Use prefix in loops (engn33r)

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

The gas savings comes from the removal of a temporary variable. The value of j++ is 1 but the value of j equals 2, which means two distinct values must be stored. In comparison, both j and ++j equal 2 when using ++j.

Proof of concept

There are 4 instances of this in Gauge.sol and 1 instance in VoteDelegation.sol

Impact

Gas savings

Recommendation

Increment with prefix addition and not postfix in for loops.

Developer Response

Fixed

4. Gas - Payable functions can save gas (engn33r)

If there is no risk of a function accidentally receiving ether, such as a function with the onlyOwner modifier, this function can use the payable modifier to save gas.

Proof of concept

The following functions have the onlyOwner modifier and can be marked as payable

setVe() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Registry.sol#L49
addVaultToRewards() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Registry.sol#L67
removeVaultFromRewards() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Registry.sol#L94
setVe() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VeYfiRewards.sol#L30
setVe() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L110
setDuration() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L73
sweep() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L111

version state variable and _version init parameter https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L155
controller() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L129-L131
transfersEnabled() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L129-L131
balanceOf() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L650
totalSupply() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L772
changeController() https://github.com/yAudit-Residents/veYFI/blob/master/contracts/VotingEscrow.vy#L831-L837

Impact

Gas savings

Recommendation

Remove unnecessary functionality copied from the original Curve contract for gas savings. This was mentioned to veYFI devs while the review was ongoing and a fix was carried out before the review concluded in PR 131, although the balanceOf() and totalSupply() functions were left unchanged.

Developer Response

Fixed

9. Gas - Inconsistent zero case in VotingEscrow.vy (engn33r)

The VotingEscrow.vy contract has two similar functions, balanceOfAt() and balanceOf(), which have been modified from the original Curve contract. The upoint.bias zero case is handled differently in the two functions. Using the lower gas solution in both would be better.

Proof of concept

The balanceOf() function uses this code:

if upoint.bias < 0:
    upoint.bias = 0
    return   convert(upoint.bias, uint256)

while the balanceOfAt() function uses this code:

if upoint.bias >= 0:
    return convert(upoint.bias, uint256)
else:
    return 0

Impact

Gas savings

Recommendation

Use the balanceOfAt() function's if/else code in balanceOf() to avoid calling convert() for the zero case.

Developer Response

Won't fix, disputed. I think this is not correct and the gas usage is the same.

10. Gas - Remove duplicated code (engn33r)

The BaseGauge.sol queueNewRewards() function contains duplicated code. This can be simplified to save on deployment and reduce code complexity. There might be slightly more gas spent on function calls where block.timestamp >= periodFinish because unnecessary math for elapsedSinceBeginingOfPeriod and distributedSoFar will be performed in this case, but deployment gas may be important because the BaseGauge.sol contract is inherited in many places.

Proof of concept

The logic for the case block.timestamp >= periodFinish and (distributedSoFar * 12) / 10 < _amount is the same, so they can be placed into the same if statement.

The revised queueNewRewards() function can look like this

function queueNewRewards(uint256 _amount) external override returns (bool) {
    require(_amount != 0, "==0");
    SafeERC20.safeTransferFrom(
        IERC20(rewardToken),
        msg.sender,
        address(this),
        _amount
    );
    emit RewardsQueued(msg.sender, _amount);
    _amount = _amount + queuedRewards;

    uint256 elapsedSinceBeginingOfPeriod = block.timestamp -
        (periodFinish - duration);
    uint256 distributedSoFar = elapsedSinceBeginingOfPeriod * rewardRate;
    // we only restart a new week if _amount is 120% of distributedSoFar.

    if (block.timestamp >= periodFinish || (distributedSoFar * 12) / 10 < _amount) {
        _notifyRewardAmount(_amount);
        queuedRewards = 0;
    } else {
        queuedRewards = _amount;
    }
    return true;
}

Impact

Gas savings

Recommendation

Use simplified code from above.

Developer Response

Won't fix. This will save deployement gas but creates and computes two variables that might not be needed.

11. Gas - SLOAD gas savings (engn33r)

The BaseGauge.sol setDuration() function can use a minor gas savings.

Proof of concept

Existing code https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L80-L84

Modified code to use local variable instead of state variable in emit

if (block.timestamp < periodFinish) {
    uint256 remaining = periodFinish - block.timestamp;
    uint256 newRate = remaining * rewardRate / newDuration;
    rewardRate = leftover;
}
duration = newDuration;
emit DurationUpdated(newDuration, newRate);

A similar type of improvement can be made in Registry.sol https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Registry.sol#L95-L96 The code can be modified to

address gauge = gauges[_vault];
require(gauge != address(0x0), "!exist");

Impact

Gas savings

Recommendation

Use modified code snippets above.

Developer Response

Fixed

Informational Findings

1. Informational - SafeERC20 functions not used (engn33r)

The ERC20 approve() function is used several times, but there can be security benefits to using the safeIncreaseAllowance() and safeDecreaseAllowance() functions instead to prevent double spend attacks. This improvement was noted in issue 69 but was not properly applied.

Proof of concept

Gauge.sol line 500 => rewardToken.approve(address(veToken), reward); Gauge.sol line 527 => IERC20(rewardToken).approve(veYfiRewardPool, toTransfer); VeYfiRewards.sol line 140 => rewardToken.approve(address(veToken), reward);

Impact

Informational. The return value is not checked so it is possible that an error occurs and the code doesn't revert when it should. The risk may be elevated if the rewardToken is an arbitrary ERC20 token.

Recommendation

Use the safeIncreaseAllowance() and safeDecreaseAllowance() functions if there is a possibility that the ERC20 tokens getting approved may not revert when an approve call fails.

Developer Response

Won't fix

2. Informational - Unspecified Voting Requirements (engn33r)

Voting on snapshot.org requires a voting strategy to be selected for each proposal, and no voting strategy is chosen for veYFI yet. veYFI is borrowing significantly from Curve and may use the same erc20-balance-of voting strategy, but there are some differences between veYFI and Curve (veYFI supports vote delegation, Curve does not) and some of the veYFI contract code makes assumptions about how the off-chain voting strategy will work. If these contract assumptions do not match how the voting strategy actually works, the off-chain voting process may not align with how the on-chain code intends.

Proof of concept

There are built-in voting assumptions in the contract code, mostly borrowed from Convex:

When a proposal is created, the checkpoint() function in VotingEscrow.vy should be called to properly set the epoch
Voting power should be calculated using the on-chain functions such as get_last_user_slope(), balanceOf(), balanceOfAt(), supplyAt(), totalSupply(), and totalSupplyAt() in VotingEscrow.vy. Note that some of the math in these calculations could be moved to an off-chain voting strategy for gas savings
Voting for a proposal with veYFI is allowed as soon as the veYFI is locked. Because there is potential for gamification in increasing veYFI stake shortly before key votes, protocols like Convex have implemented a minimum locking duration of several weeks before a token holder can vote, which is implemented in their VotingEligibility.sol contract.

Some open and unanswered questions include:

Who is allowed to create proposals on snapshot.org?
How are the results of each snapshot vote converted to on-chain rewards? Is this automated?
Will all vaults receive a gauge that can be voted for on snapshot.org, even those holding very little value? If so, this could lead to gamification of deprecated or old vaults.
Is there a maximum allocation a gauge can receive? Is there minimum quorum for governance votes? Convex specifies these values: https://docs.convexfinance.com/convexfinance/general-information/voting-and-gauge-weights

Impact

Informational. Because the off-chain voting is heavily interconnected with the on-chain contracts and unresolved questions leave risk around implementation errors in the off-chain voting proposal or strategy process.

Recommendation

Several items should be considered:

Choose an off-chain voting strategy from snapshot.org and outline all the assumptions for the voting process. veYFI is borrowing heavily from Convex, but using the same erc20-balance-of snapshot.org strategy should be thoroughly analyzed given the protocol and liquidity differences between Yearn Finance and Curve.
Determine whether the gamification risks that Convex attempts to prevent with their voting caps and voting eligibility requirements are problematic enough to add similar modifications to veYFI.
Consider automatic on-chain execution strategies to reduce centralization risk as outlined in: https://blog.aragon.org/snapshot/

Developer Response

Will fix with snapshot.org integration

3. Informational - Variable naming inconsistency (engn33r)

The first input to the createGauge() is named vault, and is passed to Gauge.sol initialize(). In initialize(), this value is named _stakingToken. If the goal is to allow gauges to correspond to liquidity pools that are NOT yearn vaults, the naming should be consistent through veYFI, otherwise future edits may be made assuming that only yearn vaults are involved. This was mentioned in a comment in issue 46.

Proof of concept

The first input to initialize() has the following comment https://github.com/yearn/veYFI/blob/d53465c5f615a04204faed55728840a1e79377fc/contracts/Gauge.sol#L67 _stakingToken The vault token to stake

Elsewhere in veYFI, the naming convention assumes that each gauge corresponds to one Yearn vault. But the name _stakingToken and the comment in issue 46 indicates this might not be correct. This should be clarified in the comments and the variable names.

Impact

Informational

Recommendation

If the intention is to limit the value of this input to only permit Yearn vaults, consider verifying whether the vault is in the list of addresses from the assetsAddresses() function of the AddressesGeneratorV2Vaults contract https://github.com/yearn/yearn-security/blob/master/SECURITY.md#production-contracts

Otherwise, if this value will not be limited to Yearn vaults, consider renaming the variables that use the word "vault" or add comments in multiple places to clarify this address may not be a yearn vault.

Developer Response

Fixed

4. Informational - Missing 0 check in `setDuration()` (engn33r)

There is no zero check in setDuration() in BaseGauge.sol. If a duration of zero is chosen, functions like _notifyRewardAmount() that divide by the duration will revert with a divide by zero error. Zero checks exist on all similar setter functions.

Proof of concept

If block.timestamp < periodFinish, then the code will check if newDuration is zero, but this check will not happen if the if statement is skipped when block.timestamp > periodFinish https://github.com/yAudit-Residents/veYFI/blob/master/contracts/BaseGauge.sol#L73

That will be done when needed.

10. Informational - Use -= to keep code concise (engn33r)

The withdraw() function of Gauge.sol can use -= to reduce code complexity.

Proof of concept

The += operator is used often in veYFI, but the -= operator is not used in this line https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L391

_balances[msg.sender] = _balances[msg.sender] - _amount;

Consider changing this line to

_balances[msg.sender] -= _amount;

Impact

Informational

Recommendation

Use -= when available for concise code.

Developer Response

Fixed

11. Informational - Update rewards for other users (engn33r)

The depositFor() function in Gauge.sol allows the _updateReward() function to be called for other depositors. This may lead to an attack where a depositor in a gauge finds a way to reduce the rewards for other depositors of the same gauge, possibly similar to the flash loan method mentioned in Medium #3, and calling depositFor() with an extremely small value to trigger _updateReward() to update the checkpoint for the other depositors. The attacker may be able to take more rewards from the gauge than they could otherwise. This was noticed at the very end of the review so examination of an attack vector was not performed.

Proof of concept

The depositFor() function: https://github.com/yAudit-Residents/veYFI/blob/master/contracts/Gauge.sol#L344

Impact

Informational

Recommendation

If there is no strong use case for depositFor(), removing this function would be the safest approach.

Developer Response

An approval mechanism has been added to the contract to whitelist addresses that has been allowed to deposit on your behalf: https://github.com/yearn/veYFI/blob/83d62f9e03ef0cc463aa31c36ce23cd3d7c184eb/contracts/Gauge.sol#L376

Final remarks

engn33r

I focused on the voting mechanics because there have been bugs here on similar projects. Once the voting implementation is fully completed and reviewed, I think veYFI will look more solid. There are some minor edge cases in the rewards system that can be ironed out, but the most critical deposit, withdraw, lock, and rewards functions look like they have been thoughtfully pieced together into a cohesive solution. Improvements to the overall documentation of the veYFI contracts and inclusion of similar protective measures to Convex are nice-to-have improvements.

NibblerExpress

I looked for ways to attack the deposit, withdraw, and reward functions to receive excess rewards or to block others from receiving rewards. I was unable to find attacks that circumvented the reward checkpointing or that manipulated the voting escrow contract. The problems seemed to be in how rewards were actually calculated. The reward formula should be reviewed to ensure that the contract is more precisely incentivizing the desired behaviors. I did not take a close look at how voting would occur outside the contract and what mischief could be caused by malicious proposals.

yAudit veYFI Review

Table of Contents

Review Summary

Scope

Code Evaluation Matrix

Findings Explanation

High Findings

1. High - Incompatible Vote Delegation (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

2. High - Lock 1e-18 YFI to Get Rewards (NibblerExpress)

Proof of concept

Impact

Recommendation

Developer Response

3. High - Flash Loan Sybil Attack to Boost Rewards (NibblerExpress)

Proof of concept

Impact

Recommendation

Developer Response

Medium Findings

1. Medium - Incorrect variables in getRewardFor call (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

2. Medium - Unclear Integration of Vote Delegation Until Value (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

3. Medium - force_withdraw() penalty may inadequately deter gamification and attacks on voting (engn33r, NibblerExpress)

Proof of concept

Impact

Recommendation

Developer Response

Low Findings

1. Low - Voting delegation is missing edge case checks (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

2. Low - Sawtooth Wave Effect from Computing veYFI Balance and Supply Using Different Time Scales (NibblerExpress)

Proof of concept

Impact

Recommendation

Developer Response

3. Low - Equal penalty reward distribution not incentive aligned (engn33r, NibblerExpress)

Proof of concept

Impact

Recommendation

Developer Response

4. Low - No penalty during migration (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

Gas Savings Findings

1. Gas - Using "unchecked" (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

2. Gas - Using simple comparison (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

3. Gas - Use prefix in loops (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

4. Gas - Payable functions can save gas (engn33r)

Proof of concept

Impact

Recommendation

Developer Response

3. Medium - `force_withdraw()` penalty may inadequately deter gamification and attacks on voting (engn33r, NibblerExpress)

4. Informational - Missing 0 check in `setDuration()` (engn33r)

← Back to Reports

veYFI

↑ Back to Top