sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

xiaoming90 - A portion of the user's bootstrapped balance is lost #66

Closed sherlock-admin3 closed 7 months ago

sherlock-admin3 commented 7 months ago

xiaoming90

high

A portion of the user's bootstrapped balance is lost

Summary

A portion of the user's bootstrapped balance is lost after a reset.

Vulnerability Detail

Assume the following example. The values chosen are intentionally simplified and magnified to demonstrate the issue, but this does not affect the validity of this issue.

image-20240319185624715

Near the end of Epoch 99 (Voting Phase), Charles voted for all proposals. Since Charles' voting power is 100, an inflation of 50% will be 50 (inflation_ = 50). In Line 117 below, the total supply will be 150 after inflation of 50 is added.

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/abstract/EpochBasedInflationaryVoteToken.sol#L105

File: EpochBasedInflationaryVoteToken.sol
105:     function _markParticipation(address delegatee_) internal onlyDuringVoteEpoch {
106:         uint16 currentEpoch_ = _clock();
107: 
108:         // Revert if could not update, as it means the delegatee has already participated in this epoch.
109:         if (!_update(_participations[delegatee_], currentEpoch_)) revert AlreadyParticipated();
110: 
111:         _sync(delegatee_);
112: 
113:         uint240 inflation_ = _getInflation(_getVotes(delegatee_, currentEpoch_));
114: 
115:         // NOTE: Cannot sync here because it would prevent `delegatee_` from getting inflation if their delegatee votes.
116:         // NOTE: Don't need to sync here because participating has no effect on the balance of `delegatee_`.
117:         _addTotalSupply(inflation_);
118:         _addVotingPower(delegatee_, inflation_);
119:     }

One epoch later (Epoch 100), a reset to power holders is triggered, and a new Power Token contract is deployed with the previous Power Token contract as the bootstrap. At Line 106, the bootstrapEpoch is set to the previous epoch (Epoch 99). At Line 107, the bootstrapSupply_ is set to the total supply of the previous epoch, which is 150.

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/PowerToken.sol#L106

File: PowerToken.sol
095:     constructor(
096:         address bootstrapToken_,
097:         address standardGovernor_,
098:         address cashToken_,
099:         address vault_
100:     ) EpochBasedInflationaryVoteToken("Power Token", "POWER", 0, ONE / 10) {
101:         if ((bootstrapToken = bootstrapToken_) == address(0)) revert InvalidBootstrapTokenAddress();
102:         if ((standardGovernor = standardGovernor_) == address(0)) revert InvalidStandardGovernorAddress();
103:         if ((_nextCashToken = cashToken_) == address(0)) revert InvalidCashTokenAddress();
104:         if ((vault = vault_) == address(0)) revert InvalidVaultAddress();
105: 
106:         uint16 bootstrapEpoch_ = bootstrapEpoch = (_clock() - 1);
107:         uint256 bootstrapSupply_ = IEpochBasedVoteToken(bootstrapToken_).pastTotalSupply(bootstrapEpoch_);
108: 
109:         if (bootstrapSupply_ == 0) revert BootstrapSupplyZero();
110: 
111:         if (bootstrapSupply_ > type(uint240).max) revert BootstrapSupplyTooLarge();
112: 
113:         _bootstrapSupply = uint240(bootstrapSupply_);
114: 
115:         _addTotalSupply(INITIAL_SUPPLY);

When Alice or Bob triggers the sync function of the new Power Token contract, the following function will be triggered:

_bootstrap() > _getBootstrapBalance(account_, bootstrapEpoch) > 

(IEpochBasedVoteToken(bootstrapToken).pastBalanceOf(account_, bootstrapEpoch) * INITIAL_SUPPLY) /_bootstrapSupply

(IEpochBasedVoteToken(bootstrapToken).pastBalanceOf(account_, Epoch_99) * 10_000) / 150

(50 * 10_000) / 150 = 3333

The issue is that in Epoch 99, both Alice and Bob's balance is 50. When Alice or Bob bootstraps their account after the reset, they will only receive 3333 PT (50/150 * 10000) instead of 5000 (Missing 1667 PT). This translates to a total of 1/3 of the initial supply being lost. Alice and Bob both lost 1667 PT each.

Alice or Bob could attempt to perform a sync on the old Power Token contract on Epoch 100 to increase their balance. However, it will not resolve this issue because the newly gained balance is added to the current epoch (Epoch 100) instead of the bootstrap epoch (Epoch 99) in the previous Power Token contract. Only the bootstrap epoch (99) is relevant after a reset.

Impact

Loss of power tokens for the affected users.

Code Snippet

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/abstract/EpochBasedInflationaryVoteToken.sol#L105

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/PowerToken.sol#L106

Tool used

Manual Review

Recommendation

During bootstrapping after a reset, the distribution of the initial supply should take into consideration of users' unrealized inflation.

nevillehuang commented 7 months ago

Invalid, known issue as a design choice, see chain security M-06

As noted in contest details

Please check individual reports and joint Audit review document to see how issues were addressed or if some of them are design choices.

xiaoming9090 commented 7 months ago

Hey @nevillehuang, I don’t agree that they are duplicates of “ChainSecurity M-06”.

The ChainSecurity M-06 report highlights that Power Tokens transferred before reset, Power Tokens obtained during the auction, and inflation Power Tokens gained from the previous voting epoch will be lost due to the reset, causing the state to rollback to Epoch N-1. These events involve losing state changes to Power Tokens that take place before a reset occurs. Issues 62 and 66 are unrelated to these three (3) concerns.

Issues 62 and 66 are different as they highlight the negative impact of the users' unclaimed inflation of Power Tokens in their accounts, resulting in a gap/deviation in their actual account balance and total supply. This gap eventually causes the users to receive fewer Power Tokens than expected when the bootstrap logic attempts to determine how much of the initial supply of Power Tokens a user should receive after a reset.

In Issue 66, Alice and Bob lost 1/3 of their power tokens after the reset due to unclaimed inflation of their accounts. If unclaimed inflation accumulates in their accounts again after the reset and a second reset is executed, they might lose another 1/3 of their power tokens. In a sense, their account’s Power Tokens balance becomes smaller after each reset whenever they have some unclaimed inflation prior to the reset. No one can expect the users to claim/expect the inflation power tokens in every epoch or whenever they earn some inflation power tokens due to gas costs. Most users will accumulate earned inflation power tokens to a certain threshold before claiming all of them at one go. So, this issue is bound to occur.

In short, this issue/risk was not highlighted in ChainSecurity M-06.

toninorair commented 7 months ago

The described situation is one of the negative side effects of resetToPower which is partially described by Chainsecurity M-06 and mentioned here in the list of acceptable risks Risk of a fraction ofINITIAL_SUPPLYwill be unallocated afterresetToPowerHoldersif it happened during Transfer epoch

xiaoming9090 commented 7 months ago

Escalate

The described situation is one of the negative side effects of resetToPower which is partially described by Chainsecurity M-06 and mentioned here in the list of acceptable risks Risk of a fraction ofINITIAL_SUPPLYwill be unallocated afterresetToPowerHoldersif it happened during Transfer epoch

  1. The reasons why ChainSecurity M-06 issue is not a duplicate of this report can be found here. The ChainSecurity M-06 issue only managed to pinpoint negative side effects correctly but failed to highlight the fact that users' unclaimed inflation of Power Tokens in their accounts, resulting in a gap/deviation in their actual account balance and total supply, could lead to the users to receive fewer Power Tokens than expected, which is the root cause of this issue. There can be multiple root causes that lead to the same side effect, but each of the root causes should be considered a unique issue and rewarded separately. In an audit contest, issues with the same root cause are duplicated together instead of the other way around, where issues with the same side effect are duplicated together.
  2. The known issue Risk of a fraction of INITIAL_SUPPLY will be unallocated after resetToPowerHolders if it happened during Transfer epoch only states that a fraction of INITIAL_SUPPLY will not be allocated to the users if a reset happens during the Transfer epoch. However, I don't see how it covers the root cause of this issue where users' unclaimed inflation Power Tokens could lead to the users receiving fewer Power Tokens than expected or fewer Power Tokens being allocated to them during the bootstrap.

Thus, this issue should remain valid.

sherlock-admin2 commented 7 months ago

Escalate

The described situation is one of the negative side effects of resetToPower which is partially described by Chainsecurity M-06 and mentioned here in the list of acceptable risks Risk of a fraction ofINITIAL_SUPPLYwill be unallocated afterresetToPowerHoldersif it happened during Transfer epoch

  1. The reasons why ChainSecurity M-06 issue is not a duplicate of this report can be found here. The ChainSecurity M-06 issue only managed to pinpoint negative side effects correctly but failed to highlight the fact that users' unclaimed inflation of Power Tokens in their accounts, resulting in a gap/deviation in their actual account balance and total supply, could lead to the users to receive fewer Power Tokens than expected, which is the root cause of this issue. There can be multiple root causes that lead to the same side effect, but each of the root causes should be considered a unique issue and rewarded separately. In an audit contest, issues with the same root cause are duplicated together instead of the other way around, where issues with the same side effect are duplicated together.
  2. The known issue Risk of a fraction of INITIAL_SUPPLY will be unallocated after resetToPowerHolders if it happened during Transfer epoch only states that a fraction of INITIAL_SUPPLY will not be allocated to the users if a reset happens during the Transfer epoch. However, I don't see how it covers the root cause of this issue where users' unclaimed inflation Power Tokens could lead to the users receiving fewer Power Tokens than expected or fewer Power Tokens being allocated to them during the bootstrap.

Thus, this issue should remain valid.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 6 months ago

Agree with sponsors comments here, this should remain invalid since it is an already considered known issue.

00001111x0 commented 6 months ago

I think there's a strong argument to be made that this issue was implicitly included when the sponsors listed the known issue, considering that the root cause of Risk of a fraction of INITIAL_SUPPLY will be unallocated after resetToPowerHolders if it happened during Transfer epoch is in fact the discrepancy stated in this issue where users' inflation is lost after bootstrapping and the total supply is larger than the sum of the user balances. I would agree with the escalation if the known issue didn't specify Transfer epoch, as the root cause could then be attributed to precision loss, but in this case it seems to me like the protocol must have been aware of this issue.

xiaoming9090 commented 6 months ago

I think there's a strong argument to be made that this issue was implicitly included when the sponsors listed the known issue, considering that the root cause of Risk of a fraction of INITIAL_SUPPLY will be unallocated after resetToPowerHolders if it happened during Transfer epoch is in fact the discrepancy stated in this issue where users' inflation is lost after bootstrapping and the total supply is larger than the sum of the user balances. I would agree with the escalation if the known issue didn't specify Transfer epoch, as the root cause could then be attributed to precision loss, but in this case it seems to me like the protocol must have been aware of this issue.

If one studies the known issue a fraction of INITIAL_SUPPLY will be unallocated after resetToPowerHolders if it happened during Transfer epoch carefully, it actually only means that any power token transferred during the transfer epoch (Epoch N) will be lost if a reset happens to be also on a same transfer epoch. The reasons behind it are stated in one of the audit reports, and that is because when the power token is bootstrapped, they use the users’ state in Epoch N-1 instead of Epoch N. Thus, any state change made (e.g., transfer, purchase) in Epoch N will be lost. This known issue is derived from that audit report issue (ChainSecurity Medium-6.2)

However, this is different from the issue here. The root cause here is that unclaimed inflation power tokens from ANY epoch in the past (not just the transfer epoch during a reset) could lead to a loss of power tokens during bootstrap. Using the above-mentioned known issue to mark this as a known issue is overstretching.

deluca-mike commented 6 months ago

I am looking at this issue, an the comments, more closely, and testing myself. Stay tuned.

deluca-mike commented 6 months ago

Ok, to be clear before getting into the meat of this, because the new PowerToken queries the old PowerToken pastBalanceOf, and pastBalanceOf uses _getBalance, which is overridden by the EpochBasedInflationaryVoteToken as super._getBalance(...) + _getUnrealizedInflation(), a user's bootstrap balance will include all unrealized (i.e. "unclaimed") inflation up until the bootstrapEpoch.

Again, to be clear, a user does not need to sync to have their past unrealized inflation be counted in the bootstrapping process.

So:

It's important to note that inflated balance is rewarded in a TransferEpoch, and just like any other way one can accumulate PowerToken (i.e. a transfer or a auction buy), accumulations (or losses) of token in the epoch the reset happens are "wiped". That just simply also includes inflations, and is to be expected. To reiterate, any reason for your PowerToken balance to increase or decreases in a epoch is ignored if a reset happens at any point in that epoch.

So this "issue" is only an "issue" if resetToPoweHolders happens in a TransferEpoch because unrealized (i.e. unsynced) inflation is rewarded in the epoch that is being ignored, while totalSupply is increased in the previous epoch. But in this case, pretty much everyone's unrealized inflation in that epoch is being ignored, and it's a flat 10%, so it affects everyone equally, and thus all old PowerToken holders will get the same percentage of the "attainable" new PowerToken. Yes, those that did not participate in the previous VotingEpoch did get spared of having their balances diluted, but it's a small price to pay for:

Further, assuming everyone participated in the previous VotingEpoch and thus the new post-reset PowerToken has ~10% of it's balance that is not distributed, this "unattainable" supply cannot vote, and will be diluted away as everyone else gains inflation.

Due tot he above, I do believe this issue is already knows, and invalid.

Evert0x commented 6 months ago

I believe the comment from the protocol team provides a thorough explanation to invalid this issue.

Planning to reject the escalation and keep the issue invalid

Evert0x commented 6 months ago

Result: Invalid Has Duplicates

sherlock-admin4 commented 6 months ago

Escalations have been resolved successfully!

Escalation status: