Yield Basis DAO Security Review

The LiquidityGauge contract uses the value of the checkpointed reward rate integral to determine the updated amount of released reward tokens, leading to multiple accounting problems.

The implementation of _get_vested_rewards() takes the value of reward_rate_integral[token].v as the amount of released rewards up to the latest checkpointed time (used_rewards).

170:     last_reward_time: uint256 = self.reward_rate_integral[token].t
171:     used_rewards: uint256 = self.reward_rate_integral[token].v
172:     finish_time: uint256 = self.rewards[token].finish_time
173:     total: uint256 = self.rewards[token].total
174:     if finish_time > last_reward_time:
175:         new_used: uint256 = (total - used_rewards) * (block.timestamp - last_reward_time) //\
176:             (finish_time - last_reward_time) + used_rewards
177:         return min(new_used, total) - used_rewards

The used_rewards variable is then used to compute the remaining amount (total - used_rewards) and to calculate the new value of distributed rewards (new_used).

However, taking the integral of the reward rate is incorrect, as this value is already scaled by the inverse of the total supply as part of the checkpoint process.

148:         r.reward_rate_integral = self.reward_rate_integral[reward]
149:         if block.timestamp > r.reward_rate_integral.t:
150:             r.reward_rate_integral.v += (r.integral_inv_supply.v - self.integral_inv_supply_4_token[reward]) * d_reward //\
151:                (block.timestamp - r.reward_rate_integral.t)
152:             r.reward_rate_integral.t = block.timestamp

Critical. The issue leads to multiple accounting problems and side effects, potentially bricking the state of the vault.

Have a dedicated accumulator to measure the amount of distributed rewards per token. This counter should be incremented by the return value of _vest_rewards() whenever the state is persisted to storage (_checkpoint_user(), claim(), deposit_reward()).

Fixed in 657e8c679cb575f35681fab810ff6615ed113924.

Rewards assigned to users are lost when the user state is checkpointed.

Unlike claim(), the implementation of _checkpoint_user() does not transfer the earned rewards to the user. However, these rewards are not stored in contract state, so they cannot be claimed later.

Critical. Earned rewards are lost when the user interacts with the vault.

The _checkpoint_user() function should store earned rewards (d_user_reward) in an accumulator of pending rewards, which can be flushed in claim(). An alternative would be to use user_rewards_integral.v which is the historic accumulated rewards, but this would also require an accumulator to track the withdrawn tokens to calculate the difference.

Added such an accumulator in 1b3b6f49c5c5140ffb2f2f5b97851e578e36f12b.

The user reward calculation is incorrectly normalized, leading to an overinflated amount.

In _checkpoint(), integral_inv_supply is scaled by 1e36 but only normalized by 1e18 when calculating d_user_reward.

139: def _checkpoint(reward: IERC20, d_reward: uint256, user: address) -> RewardIntegrals:
140:     r: RewardIntegrals = empty(RewardIntegrals)
141: 
142:     r.integral_inv_supply = self.integral_inv_supply
143:     if block.timestamp > r.integral_inv_supply.t:
144:         r.integral_inv_supply.v += unsafe_div(10**36 * (block.timestamp - r.integral_inv_supply.t), erc4626.erc20.totalSupply)
145:         r.integral_inv_supply.t = block.timestamp
146: 
147:     if reward.address != empty(address):
148:         r.reward_rate_integral = self.reward_rate_integral[reward]
149:         if block.timestamp > r.reward_rate_integral.t:
150:             r.reward_rate_integral.v += (r.integral_inv_supply.v - self.integral_inv_supply_4_token[reward]) * d_reward //\
151:                (block.timestamp - r.reward_rate_integral.t)
152:             r.reward_rate_integral.t = block.timestamp
153: 
154:     if user != empty(address):
155:         r.user_rewards_integral = self.user_rewards_integral[user][reward]
156:         if block.timestamp > r.user_rewards_integral.t:
157:             r.d_user_reward = (r.reward_rate_integral.v - self.reward_rate_integral_4_user[user][reward]) *\
158:                 erc4626.erc20.balanceOf[user] // 10**18
159:             r.user_rewards_integral.v += r.d_user_reward
160:             r.user_rewards_integral.t = block.timestamp
161: 
162:     return r

Critical. Reward amounts are inflated by a factor of 1e18.

In d_user_reward, divide by 10**36 instead of 10**18.

Fixed in dbc12194da1971ba7a32c459374b9de05a428787.

A reentrancy in safeTransferFrom() would allow an attacker to use their voting power just before being merged with the recipient.

The implementation of safeTransferFrom() does the transfer with the callback after performing the check but before clearing the owner's voting power.

538: def safeTransferFrom(owner: address, to: address, token_id: uint256, data: Bytes[1_024] = b""):
539:     assert erc721._is_approved_or_owner(msg.sender, token_id), "erc721: caller is not token owner or approved"
540:     assert self._ve_transfer_allowed(owner, to), "Need max veLock"
541:     erc721._safe_transfer(owner, to, token_id, data)
542:     self._merge_positions(owner, to)
543:     erc721._burn(token_id)

This would allow a malicious user to exercise their voting power in between the transfer, as the check is done before the callback and their votes are reset after the callback.

Critical. Voting power can be reused multiple times by performing the described attack.

Since the token is immediately burned after transferring, the underlying transfer operation is not really needed, the token can be just burned.

Fixed for both transferFrom() and safeTransferFrom() in 4d6cd7183e39487a80dc51b7976099b156f440a0.

LiquidityGauge.deposit_reward() incorrectly calculates the new finish time when adding rewards to an ongoing distribution period, resulting in unintended changes to the reward distribution rate.

LiquidityGauge.deposit_reward() contains logic to extend the finish time when adding rewards to maintain the current rate:

# Keep the reward rate
assert r.finish_time > block.timestamp, "Rate unknown"
r.finish_time = block.timestamp + (r.finish_time - block.timestamp) * (r.total + amount) // r.total

However, this formula uses the cumulative total deposited tokens instead of the remaining undistributed tokens, causing the distribution rate to change:

Example scenario:

• Initial: 1000 tokens over 10 days → rate = 100 tokens/day
• r.total = 1000 (cumulative total)
• After 6 days: 600 tokens distributed, 400 remaining
• Add 500 tokens using current formula:
  • remaining_time = 4 days
  • r.total + amount = 1000 + 500 = 1500
  • ratio = 1500 / 1000 = 1.5
  • new_remaining_time = 4 * 1.5 = 6 days
  • New rate = 900 / 6 = 150 tokens/day

The formula should use remaining tokens (400) instead of cumulative total (1000) to maintain the same rate:

• Correct ratio = (400 + 500) / 400 = 2.25
• Correct new time = 4 * 2.25 = 9 days
• Correct rate = 900 / 9 = 100 tokens/day

High. The reward distribution rate changes unexpectedly when adding tokens to ongoing distributions, violating the stated intention to "keep the reward rate".

Calculate the extension based on the original rate to maintain consistent distribution: block.timestamp + (undistributed_reward + amount) / reward_rate.

Fixed in b5a9150825ae440e77a512f90ed3a44d126c63ad.

When checkpointing a specific gauge, the global emission rate calculation uses potentially stale weights from other gauges that haven't been recently checkpointed.

GaugeController._checkpoint_gauge() updates weights only for the target gauge, but then uses global weight sums (aw_sum, w_sum) to calculate the emission rate factor.

If other gauges have changed their weights but haven't been checkpointed, their stale weights are still included in the global sums, leading to incorrect emission rate calculations.

High. Incorrect emission rates affect the entire protocol's token distribution, potentially over-minting or under-minting tokens based on outdated weight information.

Implement global gauge aggregation and update it during gauge checkpoints, similar to Curve's _get_total() approach, adapted for Yield Basis with no gauge types.

Disagree. That's the thing. Weights are only ever updated with per-gauge checkpoint.

That checkpoint happens at voting as well as at claim. But if gauge, for example, had adjustment number changed somehow - a stale number is applied all across the board before chekpoint or claim happens.

It is by design. So weights CANNOT actually update without being checkpointed. Disadvantage of this approach is that weights cannot be a pure function of time: they are only updated in actions which cause checkpoints. But this is not a big disadvantage because timescale of vote weight changes (years) is much larger than times between checkpoints (e.g. between claims, or deposits/withdrawals).

VotingEscrow.increase_amount() has a critical parameter inconsistency that creates a mismatch between validation and execution.

VotingEscrow.increase_amount() performs validation checks against msg.sender's lock data but then calls _deposit_for(_for, ...), which modifies the lock for the _for address. This creates a scenario where the actual token deposit and lock modification occurs for the _for address, while the validation is done with msg.sender's lock data and the _locked parameter passed to _deposit_for contains msg.sender's lock data, not _for's.

High. This vulnerability allows:

• Users to call the function for addresses that don't have existing locks
• Bypassing validation that the target lock is valid and non-expired
• Creating inconsistent lock states where the wrong lock parameters are used
• Potential manipulation of the voting escrow system by depositing for invalid targets

Fix the parameter inconsistency by ensuring validation and operation target the same address:

    @external
    @nonreentrant
    def increase_amount(_value: uint256, _for: address = msg.sender):
-       _locked: LockedBalance = self.locked[msg.sender]
+       _locked: LockedBalance = self.locked[_for]

        assert _value > 0  # dev: need non-zero value
        assert _locked.amount > 0, "No existing lock found"
        assert _locked.end > block.timestamp, "Cannot add to expired lock. Withdraw"

        self._deposit_for(_for, _value, 0, _locked, LockActions.INCREASE_AMOUNT)

Or alternatively, remove the _for parameter if it's not needed.

Fixed in 80137af5ce7e969f2a10d254b0a05756f3c2c6ce.

The emission rate in the Yield Basis token is incorrectly scaled.

The emissions calculation is given by the implementation of _emissions():

50: def _emissions(t: uint256, rate_factor: uint256) -> uint256:
51:     assert rate_factor <= 10**18
52:     dt: int256 = convert(t - self.last_minted, int256)
53:     rate: int256 = convert(max_mint_rate * rate_factor // 10**18, int256)
54:     reserve: int256 = convert(self.reserve, int256)
55:     return convert(
56:         reserve * (10**18 - math._wad_exp(-dt * rate // 10**18)) // 10**18,
57:         uint256)

The rate variable is calculated as the max_mint_rate (max rate per second) scaled by the rate_factor. However, the rate variable is then normalized again by 10**18 in the expression of -dt * rate // 10**18 (line 56). Given dt and rate are denominated in seconds, dividing by 10**18 will yield the incorrect result.

High. Emission rate in YB token is incorrect and will likely result in zero emissions.

The exponential term should multiply dt (elapsed seconds) by rate (emissions per second).

reserve * (10**18 - math._wad_exp(-dt * rate)) // 10**18

Fixed in 14ff742f630a64f786beb5589ee978f541950768.

LiquidityGauge.withdraw() contains an incorrect assert statement that validates the wrong account's withdrawal capacity, causing legitimate withdrawal operations to fail when transferring assets to third parties.

In LiquidityGauge.withdraw() the assert checks _max_withdraw(receiver) instead of _max_withdraw(owner). The validation should ensure the owner has sufficient withdrawable balance, not the receiver.

Medium. This bug breaks core functionality, but severity is mitigated because redeem() is correct and can be used instead.

Fix the assert to validate the owner's withdrawal capacity instead of the receiver's.

Fixed in 58ca825891930e069ad1a1f2c82581399a5004fa.

Using the LP token as a reward would conflict with the vault's accounting.

LP tokens sent by the distributor would be mixed with staked tokens and treated as the vault's assets, disrupting the accounting.

Low.

Check the reward token is not the LP token in add_reward().

    def add_reward(token: IERC20, distributor: address):
        assert token != YB, "YB"
+       assert token != LP_TOKEN, "LP_TOKEN"

Fixed at 395d1db03e4a9aa892642022c794a7ef9c066105.

The ealy return in preview_emissions() implements the wrong condition.

The implementation of preview_emissions() early returns with zero using the wrong condition. A gauge isn't registered when time_weight[gauge] == 0, a positive value means the gauge has been enabled.

325:     if self.time_weight[gauge] > 0:
326:         return 0

Low. preview_emissions() always returns zero for registered gauges.

Change the condition to if self.time_weight[gauge] == 0.

Fixed in 7192d1b8f7f7d303c3fd6fe39ae2358591ff995b.

There are severals functions inherited from modules which are not re-exported from the contract.

• GaugeController.vy:
  • owner()
• VestingEscrow.vy:
  • owner()
• VotingEscrow.vy:
  • tokenURI()
  • supportsInterface()

Informational.

Add the missing exports to expose the functions.

Fixed in 199646175b42a081f049f1a26e3362c4fa878450.

VestingEscrow.fund() accepts two arrays but only validates the loop bounds against the recipients array.

Informational.

Add explicit length validation for better error messaging:

assert len(_recipients) == len(_amounts), "Array length mismatch"

Fixed in 926621b0e79d8f2c08d7f69f2d24e9b6f02447a7.

VotingEscrow.getPastVotes() allows querying voting power at any timestamp without proper validation. However, the function's purpose is to return voting power from the past only.

Informational.

Add proper timepoint validation check:

assert timepoint <= block.timestamp, "Timepoint in the future"

Acknowledged. It's a view method which is to be called from frontend. Sometimes RPCs are using multiple nodes behind a load balancer which could be not entirely at sync (one can be ahead of another by 1 block). So, I can imagine a situation that check <= block.timestamp will actually fail after reading the current timestamp worked. Moreover, this function DOES work for the nearest future as well. Overall, I think better leave it without this assert.

The CliffEscrow.vy contract contains a spelling error in an immutable variable name:

RECEPIENT: public(immutable(address))

Informational.

Fix typos.

Fixed in d3dd301fe26e47cf575b844c4bd2a2c6efbc50b2.

Some functions updates the contract state after making an external call, violating the Checks-Effects-Interactions (CEI) pattern.

VotingEscrow.withdraw() performs the state update erc721._burn(convert(msg.sender, uint256)) before making the external call to TOKEN.transfer().

LiquidityGauge.deposit_reward() performs the state update self.rewards[token] = r before making the external call to token.transferFrom().

Informational.

Follow CEI pattern as a best practice.

Fixed in 7c28c5b3cff00a71b5c598507b99ea4a8e16e389.

In VotingEscrow._merge_positions() two variables pt and to_pt are declared and assigned but never used, resulting in unnecessary gas consumption.

Gas savings.

Remove the unused variables.

Fixed in cdf753d798f7bb8ca2f7c894ee50a54408878072.