search

sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

0xpiken - The sum of all voting power might be different from the sum of all power token balance due to the flaw of inflation mechanism #38

Closed sherlock-admin2 closed 5 months ago

sherlock-admin2 commented 5 months ago

0xpiken

medium

The sum of all voting power might be different from the sum of all power token balance due to the flaw of inflation mechanism

Summary

The sum of all voting power might be different from the sum of all power token balance due to the flaw of inflation mechanism

Vulnerability Detail

The total supply of Power Token will be inflated if there is at least one active standard proposal in the voting epoch. A voter can inflate their own Power balance as well as the Power balances of all delegators by voting on all active proposals. The sponsor clearly declared the Main Invariants in their TTG spec:

  1. π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘‰π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘†π‘’π‘π‘π‘™π‘¦hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘ 
  2. βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  >= βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‰π‘œπ‘‘π‘–π‘›π‘” π‘’π‘π‘œπ‘h
  3. βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‡π‘Ÿπ‘Žπ‘›π‘ π‘“π‘’π‘Ÿ π‘’π‘π‘œπ‘h
  4. π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘1 = π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘0 + πΌπ‘›π‘“π‘™π‘Žπ‘‘π‘–π‘œπ‘›π‘–π‘›π‘Žπ‘π‘‘π‘–π‘£π‘’π‘ƒπ‘Žπ‘Ÿπ‘‘π‘–π‘π‘–π‘π‘Žπ‘›π‘‘π‘ 

However, item 3 can be broken due to precision loss when calculating inflation. The inflation rate of Power Token is set to 10% by the sponsor. Here is an example illustrating how the inflation problem occurs:

Update Integration.t.sol to change initial balances firstly:

-   uint256[] internal _initialPowerBalances = [55, 25, 20];
+   uint256[] internal _initialPowerBalances = [5505, 2495, 2000];

Then copy below codes to Integration.t.sol and run forge test --match-test test_SumOfAllBalanceNotEqualToSumOfAllVotingPower

    function test_SumOfAllBalanceNotEqualToSumOfAllVotingPower() external {
        IStandardGovernor standardGovernor_ = IStandardGovernor(_registrar.standardGovernor());
        IPowerToken powerToken_ = IPowerToken(_registrar.powerToken());

        _warpToNextTransferEpoch();
        //@audit-info Alice delegates her voting power to Bob
        vm.prank(_alice);
        powerToken_.delegate(_bob);

        address[] memory targets_ = new address[](1);
        targets_[0] = address(standardGovernor_);

        uint256[] memory values_ = new uint256[](1);

        bytes32 key_ = "TEST_KEY";
        bytes32 value_ = "TEST_VALUE";

        bytes[] memory callDatas_ = new bytes[](1);
        callDatas_[0] = abi.encodeWithSelector(standardGovernor_.setKey.selector, key_, value_);

        string memory description_ = "Update config key/value pair";

        uint256 proposalFee_ = standardGovernor_.proposalFee();

        _cashToken1.mint(_alice, proposalFee_);

        vm.startPrank(_alice);
        _cashToken1.approve(address(standardGovernor_), proposalFee_);
        uint256 proposalId_ = standardGovernor_.propose(targets_, values_, callDatas_, description_);
        vm.stopPrank();
        assertEq(_cashToken1.balanceOf(_alice), 0);
        assertEq(_cashToken1.balanceOf(address(standardGovernor_)), proposalFee_);

        _warpToNextVoteEpoch();

        vm.prank(_bob);
        standardGovernor_.castVote(proposalId_, 1);
        vm.prank(_carol);
        standardGovernor_.castVote(proposalId_, 1);

        _warpToNextTransferEpoch();

        uint totalSupply = powerToken_.balanceOf(_alice) + powerToken_.balanceOf(_bob) + powerToken_.balanceOf(_carol);
        uint totalVotingPower = powerToken_.getVotes(_alice) + powerToken_.getVotes(_bob) + powerToken_.getVotes(_carol);
        //@audit-info the sum of all power token balance is not equal to the sum of all voting power
        assertNotEq(totalSupply, totalVotingPower);
        vm.prank(_alice);
        powerToken_.delegate(address(0));
        vm.startPrank(_bob);
        powerToken_.transfer(_alice, powerToken_.balanceOf(_bob));
        vm.stopPrank();
        //@audit-info Bob has no any delegaotr and power token, but he has 1 voting power.
        assertEq(powerToken_.balanceOf(_bob), 0);
        assertEq(powerToken_.getVotes(_bob), 1);
    }

Impact

The invariant that βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‡π‘Ÿπ‘Žπ‘›π‘ π‘“π‘’π‘Ÿ π‘’π‘π‘œπ‘h is broken

Code Snippet

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

Tool used

Manual Review

Recommendation

Finding a solution to this problem seems incredibly challenging because precision loss can occur anytime.

PierrickGT commented 5 months ago

The reasoning seems valid but I wasn't able to reproduce the error described in the provided test: https://github.com/MZero-Labs/ttg/pull/263/commits/0864117e60de472c4de08af52f4f13aa34b01ae1

nevillehuang commented 5 months ago

request poc

sherlock-admin3 commented 5 months ago

PoC requested from @piken

Requests remaining: 4

piken commented 5 months ago

The balances of Alice and Bob have been inflated to 5500 and 2500 from the beginning in the above test - 0864117.

Please update all initial balances in IntegrationBaseSetup.t.sol to avoid bootstrap inflation:

    uint256[][2] internal _initialBalances = [
-       [uint256(55), 25, 20],
+       [uint256(5505), 2495, 2000],
        [uint256(60_000_000e6), 30_000_000e6, 10_000_000e6]
    ];
deluca-mike commented 5 months ago

While a valid finding, this is unavoidable. There is simply no way to achieve perfection in the invariants due to rounding of integer math. The invariants specified were purely mathematical. The actual invariants are <= rather than ==.

We won't fix because we can't fix, but I debate if this is actual an issue since it applies to such a wide array of contracts out there.

nevillehuang commented 5 months ago

@deluca-mike Since this was mentioned as an explicit invariant, I believe medium severity to be appropriate

PierrickGT commented 5 months ago

The balances of Alice and Bob have been inflated to 5500 and 2500 from the beginning in the above test - 0864117.

Please update all initial balances in IntegrationBaseSetup.t.sol to avoid bootstrap inflation:

    uint256[][2] internal _initialBalances = [
-       [uint256(55), 25, 20],
+       [uint256(5505), 2495, 2000],
        [uint256(60_000_000e6), 30_000_000e6, 10_000_000e6]
    ];

I did and tests were still passing.

piken commented 5 months ago

@PierrickGT The test did pass because somehow the power token balances were inflated to [550500, 249500, 200000] under commit 0864117. May you try the test case under auditing version?

piken commented 5 months ago

@PierrickGT I believe I found the reason. PowerToken#INITIAL_SUPPLY has been changed to 1_000_000. Please update all initial balances as below to avoid inflation:

    uint256[][2] internal _initialBalances = [
-       [uint256(55), 25, 20],
+       [uint256(550005), 249995, 200000],
        [uint256(60_000_000e6), 30_000_000e6, 10_000_000e6]
    ];
toninorair commented 5 months ago

This behavior was already mentioned in previous audits and is an unavoidable nature of rounding math logic. It was accepted as expected behavior due to rounding issues.

nevillehuang commented 5 months ago

@toninorair Where was it highlighted specifically that it was mentioned in previous audits?

toninorair commented 5 months ago

@nevillehuang we don't highlight specifically every single issue, but asking to exclude the ones that were already reported by other auditors.

PierrickGT commented 5 months ago

@nevillehuang it is highlighted in the Prototech report, Section 8 High Risks, issue 8.3: https://github.com/MZero-Labs/sherlock-contest-docs/blob/main/audit-reports/Prototech%20Labs%20Audit%20Report.pdf

nevillehuang commented 5 months ago

See also chainsecurity issue 6.6

  1. Holders of PowerToken with less than 10 PowerTokens do not get any balance inflation although they (or their delegatee) vote in all proposals. This could happen due to the rounding down in function _getInflation(). In case the holder delegates to another account which has a voting power more than 10, its voting power inflates, but the balance of the delegator will not inflate
kadenzipfel commented 5 months ago

Escalate

This should be marked as a valid issue for the following reasons:

1) The finding clearly demonstrates how a "Main Invariant", as described by a technical specification included in the contest readme, is broken. See in the issue above:

The sponsor clearly declared the Main Invariants in their TTG spec:

π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘‰π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘†π‘’π‘π‘π‘™π‘¦hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  >= βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‰π‘œπ‘‘π‘–π‘›π‘” π‘’π‘π‘œπ‘h βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‡π‘Ÿπ‘Žπ‘›π‘ π‘“π‘’π‘Ÿ π‘’π‘π‘œπ‘h π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘1 = π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘0 + πΌπ‘›π‘“π‘™π‘Žπ‘‘π‘–π‘œπ‘›π‘–π‘›π‘Žπ‘π‘‘π‘–π‘£π‘’π‘ƒπ‘Žπ‘Ÿπ‘‘π‘–π‘π‘–π‘π‘Žπ‘›π‘‘π‘ 

2) The referenced Prototech 8.3 audit finding is marked as "Resolved" by the sponsor. As such, since it is proven above why the finding is not in fact resolved, it should be valid.

Additionally, this finding should be marked as high severity for the following reasons as described in duplicate https://github.com/sherlock-audit/2023-10-mzero-judging/issues/31:

1) The PowerToken has an initial supply of 10000 tokens with 0 decimals. This means that any user with less than 0.01% of the token supply (<10 tokens) cannot receive inflation.

2) This vulnerability causes Ξ£ POWER balanceOf(delegators) < POWER votingPower(delegatee). The result is that the delegatee has a non-zero voting power even if there is no one voting for them. This problem compounds over time and with many accounts delegating to them, and since inflation is continuously compounding, the delegatee will have an increasing range between votingPower and actual amount of delegated votes. Note also that this impact is not stated in any prior finding.

sherlock-admin2 commented 5 months ago

Escalate

This should be marked as a valid issue for the following reasons:

1) The finding clearly demonstrates how a "Main Invariant", as described by a technical specification included in the contest readme, is broken. See in the issue above:

The sponsor clearly declared the Main Invariants in their TTG spec:

π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘‰π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == π‘ƒπ‘‚π‘ŠπΈπ‘… π‘‘π‘œπ‘‘π‘Žπ‘™π‘†π‘’π‘π‘π‘™π‘¦hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  >= βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‰π‘œπ‘‘π‘–π‘›π‘” π‘’π‘π‘œπ‘h βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘£π‘œπ‘‘π‘–π‘›π‘”π‘ƒπ‘œπ‘€π‘’π‘Ÿπ‘‘π‘’π‘™π‘’π‘”π‘Žπ‘‘π‘’π‘  == βˆ‘π‘ƒπ‘‚π‘ŠπΈπ‘… π‘π‘Žπ‘™π‘Žπ‘›π‘π‘’π‘‚π‘“hπ‘œπ‘™π‘‘π‘’π‘Ÿπ‘  , π‘Žπ‘‘ π‘‡π‘Ÿπ‘Žπ‘›π‘ π‘“π‘’π‘Ÿ π‘’π‘π‘œπ‘h π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘1 = π‘Žπ‘šπ‘œπ‘’π‘›π‘‘π‘‡π‘œπ΄π‘’π‘π‘‘π‘–π‘œπ‘›π‘‘0 + πΌπ‘›π‘“π‘™π‘Žπ‘‘π‘–π‘œπ‘›π‘–π‘›π‘Žπ‘π‘‘π‘–π‘£π‘’π‘ƒπ‘Žπ‘Ÿπ‘‘π‘–π‘π‘–π‘π‘Žπ‘›π‘‘π‘ 

2) The referenced Prototech 8.3 audit finding is marked as "Resolved" by the sponsor. As such, since it is proven above why the finding is not in fact resolved, it should be valid.

Additionally, this finding should be marked as high severity for the following reasons as described in duplicate https://github.com/sherlock-audit/2023-10-mzero-judging/issues/31:

1) The PowerToken has an initial supply of 10000 tokens with 0 decimals. This means that any user with less than 0.01% of the token supply (<10 tokens) cannot receive inflation.

2) This vulnerability causes Ξ£ POWER balanceOf(delegators) < POWER votingPower(delegatee). The result is that the delegatee has a non-zero voting power even if there is no one voting for them. This problem compounds over time and with many accounts delegating to them, and since inflation is continuously compounding, the delegatee will have an increasing range between votingPower and actual amount of delegated votes. Note also that this impact is not stated in any prior finding.

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 5 months ago

@kadenzipfel Check chainsecurity issue 6.6, in both the google docs (where it is marked as won't fix) and the full audit where the issue was more thoroughly explained. Is the invariant mentioned the same one mentioned here?

kadenzipfel commented 5 months ago

@nevillehuang, I think the problem here is that we have a circumstance where we have a clash between a clearly defined invariant which is intended to be validated by Watson's and a case where the mechanism to break said invariant is also a known issue. Note that the ChainSecurity report didn't identify this invariant, likely because the invariants listed in the TTG spec were different for the previous audits (they were corrected during this contest). So the vulnerability is known, but the vulnerability is not known to break the invariant which is intended to be validated against in this contest. As far as I can tell, there's no clear ruling in the docs as to how to judge this kind of dilemma. The way I see it, failing to award this finding sets a precedent for weighting known issues higher than invariants which prevents the sponsor from having findings like this reported.

On that note, for the sake of the protocol, I'd like the sponsor to reconsider fixing this issue based on the impact and recommendation from this duplicate finding: https://github.com/sherlock-audit/2023-10-mzero-judging/issues/31

nevillehuang commented 5 months ago

@kadenzipfel I agree with you, I will delegate this to @Czar102, since I believe given the impact of #31, this issue is worthy to fix although might come with certain complexities.

pkqs90 commented 5 months ago

Hi, apologies, but I'd like to add my perspective on this issue. Although it contradicts an invariant from a protocol document, I believe this issue should not be validated due to its root cause being identical to Chainsecurity's issue 6.6 (Note that it states both impacts: 1. Holders of PowerToken with less than 10 PowerTokens do not get any balance inflation and 2. the difference between the total supply and the actual sum of all account balances grows over time, and might have implications in the quorum-based governance if threshold is high), which the protocol team has marked as 'risk accepted.'

Additionally, the contest's readme explicitly mentions These and other already described by Auditors issues with responded and acknowledged status. Especially INFO issues from Audit review document. under Q: Please list any known issues/acceptable risks that should not result in a valid finding, which actually prevented me from submitting this issue. From my point of view, for these marginal issues, they may be categorized into low/informational severity (which seems Sherlock now supports?)

00001111x0 commented 5 months ago

Should this issue should be considered as Low? The broken invariant described here does not cause loss of funds or break protocol functionality in and of itself.

Contrast with #31, which I think shows loss of funds/functionality (H/M impact).

nevillehuang commented 5 months ago

@00001111x0 Could be argued that way yes, but could also argue that this issue actually has an absolute proof of the issue with a workable coded PoC.

deluca-mike commented 5 months ago

Just want to chime in here. The Power Token's initial supply is a balance between how many discrete supply inflations can occur before overflowing and how small a balance of the "bootstrap token" before you are diluted out entirely after a reset. The design choice is for longevity. In our latest code, we've since increased the initial supply to 1_000_000, but mostly as a response to trying to best satisfy dispersion in the initial (inaugural) bootstrap to a set of predetermined accounts. Even if we took suggestions to increase the initial supply to 10_000e18 (which we won't do), there would still be truncation, and thus total supply will never equal the sum of all voting power, adn the sum of all voting power will never equal the sum of all balance. The larger values will always be slightly more (i.e. totalSupply >= sum of all voting power, sum of all voting power >= sum of all balances).

The invariants were expressed mathematically, but in practice, due to integer rounding (which every Solidity contract deals with), we need to accept the fact that math on smaller values results in relatively more truncation loses than math on larger values. But what is important is that the there is safety.

31 is equally invalid as the recommendation/suggestion would still result in a broken invariant if you don't translate the mathematical invariants into integer-math and accept that balances will always be trucnated more than voting powers, and voting powers will always be truncated more that total supply.

kadenzipfel commented 5 months ago

Even if we took suggestions to increase the initial supply to 10_000e18 (which we won't do), there would still be truncation, and thus total supply will never equal the sum of all voting power, adn the sum of all voting power will never equal the sum of all balance. The larger values will always be slightly more (i.e. totalSupply >= sum of all voting power, sum of all voting power >= sum of all balances).

The important part to consider here is that the difference between a 1/10000 rounding loss and a 1/10000e18 rounding loss is massive. The reason 18 decimal fixed point math is so common is because the rounding errors incurred are completely insignificant. So while it may be the case that the invariant isn't exactly correct with the mitigation, it would be incorrect by a matter of dust amounts rather than a significant portion of the total supply.

deluca-mike commented 5 months ago

The important part to consider here is that the difference between a 1/10000 rounding loss and a 1/10000e18 rounding loss is massive

Yes, and while I agree, this now falls under the umbrella of design decisions. Having a healthy Power Token last for decades with the possibility of 10% inflation every 30 days was deemed more important that the rounding errors that:

And to be clear, I am not even saying that I personally deem the specific tradeoff of an initial supply of 10_000 or 1_000_000 to be worth it, but just that the company and stakeholders have as a whole.

The conversation seems to have shifted away from "the mathematical equality in an invariant is broken when you do integer math" (which, would always be the case, even at an initial supply of 10_000e18), to "subjectively, what amount of truncation are we ok with?".

Evert0x commented 5 months ago

I believe the escalation should be rejected and the issue should stay invalid.

The issue mentioned in the report can never be resolved as rounding loss is a side effect of the EVM. The invariant is not being disrespected with that context in mind.

Evert0x commented 5 months ago

Result: Invalid Has Duplicates

sherlock-admin4 commented 5 months ago

Escalations have been resolved successfully!

Escalation status: