Issue M-21 from Code4rena audit not correctly fixed

[hats-bug-reporter[bot]]

Github username: @bauchibred Twitter username: bauchibred Submission hash (on-chain): 0xcf89c2eb7afa8d2b055ba3b314bb5359193694c4fc289f7828911efec425992f Severity: medium

Description: Description

See link.

This was probably missed as it was previously duplicated to a different issue.

Take a look at https://github.com/hats-finance/Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad/blob/5da08b3d97da6d1989d73892ccabfe7438ae8a9d/contracts/governance/twTAP.sol#L626C1-L632C1

        // Remove participation
        if (position.hasVotingPower) {
            TWAMLPool memory pool = twAML;
            unchecked {
                --pool.totalParticipants;
            }

The above snippet is from the function twTAP._releaseTap() which updates only --twAML.totalParticipants and neglects to update twAML.averageMagnitude. This will result in twAML.averageMagnitude accumulating every time a new position participates, without ever decreasing.

Attack Scenario

TLDR of the report is that an attacker can monopolize the governance by precisely controlling the number of tokens entering and exiting to ensure only their position remains in the current twTap. Additionally, if pool.cumulative is less than pool.averageMagnitude, it will be set to 0, but position.averageMagnitude will be added to pool.cumulative upon exit, causing it to continuously increase. This results in the twTAP/TapiocaOptionBroker's efficiency decreasing, as their multiplier or target will always be at the minimum value. The same issue exists in the TapiocaOptionBroker, but pool.cumulative will reset to EPOCH_DURATION instead of zero. This problem can be observed in the twTap exploit case. Recommendation Apply the fix as was suggested in M-21, i.e update both the values for --twAML.totalParticipants and twAML.averageMagnitude in releaseTwap(). Attachments

1. Proof of Concept (PoC) File

See link for more info.

2. Revised Code File (Optional)

N/A

[0xRektora]

This issue happens when the attack is the first and only participant, by participating, then exiting the pool. The aM is greater than the cumulative, the pool is reset,

This scenario means there would be no users for the whole EPOCH_DURATION, which is set for a week. This is the intended behaviour, since there are no users, the pool will just reset to being pool.cumulative = EPOCH_DURATION;, which after a new participation, the aM would be updated using new variables (1) pool.averageMagnitude = (pool.averageMagnitude + magnitude) / pool.totalParticipants; // compute new average magnitude. However, in a case where the cumulative reset, we'd want to also reset the aM as well.

The other type of scenario could never happen because of the formula (1) getting smaller and smaller as the number of participants increase. Which with that in mind, aM > C could never happen.

Even if the pool was broken with the scenario where only the first participant gets to attack the cumulative, the damage would only reflect on the next participant, which would only make it so the aM of the participant will not be accounted for. However the aM is fixed on the second participant, because of the formula (1), which would make the cumulative bigger than the aM.

Because the likelihood of this scenario is very low, we're labeling it as invalid, except if demonstrated otherwise.

[Bauchibred]

Hi @0xRektora, thanks for the comment, however I don't think we can take the stance that the rate at which formula (1) is getting smaller to be the same with the rate at which aM is increasing.

Attaching the previous POC here for easy access

>NB: Attached code snippet is from the current scope. We describe this attack using the simplest scenario, assuming that initially, the pool contains only the attacker's own position, but the pool.averageMagnitude is not zero(which means there were some participators before). 1. The attacker first needs to create a positions with `divergenceForce = true`, and `magnitude = M1 = current pool.averageMagnitude`. After the participation, the `pool.averageMagnitude` will be `pool.averageMagnitude +M1`. 2. Waiting for the unlock time and exits the above position. After the exit, pool.averageMagnitude remains unchanged(pool.averageMagnitude +M1), but the pool.totalParticipants is back to 0. 3. The attacker repeats the above process, which results in pool.averageMagnitude increasing by M1 each time. 4. Before the last unlock, the `pool.cumulative` and `pool.averageMagnitude` now are very large values, the attacker participates with `divergenceForce = false` for a couple of months. 5. Then the attacker exits his last position with `divergenceForce = true`. It will make `pool.cumulative = 0;`, which breaks the developers' assumptions about this branch: https://github.com/hats-finance/Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad/blob/5da08b3d97da6d1989d73892ccabfe7438ae8a9d/contracts/governance/twTAP.sol#L361-L366 ```solidity // TODO: Strongly suspect this is never less. Prove it. if (pool.cumulative > pool.averageMagnitude) { pool.cumulative -= pool.averageMagnitude; } else { pool.cumulative = 0; } ``` Now others will no longer be able to participate in twTAP, and the gov will be monopolized by the attacker. Please note that "the pool contains only the attacker's own position" is not a necessary condition; the attack can still be carried out even with the participation of others. As the exit process accumulates, this attack will become increasingly easier to occur, and may even be triggered unintentionally by users.

Note that the attack from the linked report includes having the attacker participating with a position of  divergenceForce = true, and magnitude = M1 = current pool.aM. After the participation, the pool.aM will be pool.aM +M1 which means the previous pool.aM has been doubled, now since we are using the simplistic scenario as hinted from the POC above, where it’s only the attacker’s position, this means that after their exit the total participants value is now back to zero, on the next attempt of the attacker’s participation, formular (1) doesn’t get smaller, cause the divisor is now 1 since pool.totalParticipants would be one, having only the attacker participating, this imo showcases how with every instance of the attacker’s participation the pool.aM is actually going to get incremented by M1 and actually never decreased.


I believe this logic is also applicable if we consider there were previously more than one participant before the attacker, let’s use a random value, say 3, using a similar attack pattern, the attacker participates with  divergenceForce = true, and magnitude = X = current pool.aM, which doubles the previous pool.aM from X to 2X , now formular (1) would actually get bigger, cause we are going from pool.aM = X/3 to pool.aM =2X/4 = X/2, and X/2 > X/3.



To finalize, I think we shouldn't conclude that there is an absolute direct relation that "the more the participants, the smaller formula (1) gets", cause we have to factor in that depending on the magnitude the new participant is participating with, the new average magnitude after participating could actually be bigger, showcasing how this attack is still viable.

[0xRektora]

Hey @Bauchibred ! Let's remove the scenario where there's only 1 participant because this would not realistically be possible, instead, let's focus on the one where participants are already present.

Here's a Google colab link where you could play with some different type of simulations (growing, stagnating, decaying).

There has been a small error describing aM, it can be lower or higher than C given a time t. However the system works as an arrow, where if the magnitude/duration is greater than the C, the system would grow, and if not, the system decays.

            divergenceForce = _duration >= pool.cumulative;
            if (divergenceForce) {
                pool.cumulative += pool.averageMagnitude;
            } else {
                // TODO: Strongly suspect this is never less. Prove it.
                if (pool.cumulative > pool.averageMagnitude) {
                    pool.cumulative -= pool.averageMagnitude;
                } else {
                    pool.cumulative = EPOCH_DURATION;
                }
            }

Mathematically speaking (1) can be bigger than C, which signify a growth phase. However because C is always an aggregation of all aM, at release, we just subtract the user's aM at the time of release, which makes the invariant pass.

Given your hypothesis

I believe this logic is also applicable if we consider there were previously more than one participant before the attacker, let’s use a random value, say 3, using a similar attack pattern, the attacker participates with divergenceForce = true, and magnitude = X = current pool.aM, which doubles the previous pool.aM from X to 2X , now formular (1) would actually get bigger, cause we are going from pool.aM = X/3 to pool.aM =2X/4 = X/2, and X/2 > X/3.



We should take into account that divergenceForce == true; C += pool.aM(1x); C+= pool.aM(2x), which at release will just be the inverse divergenceForce == true; C -= pool.aM(1x); C-= pool.aM(2x), which retain the "solvency" of C.

The only way for this to break as of now, with the way we understand it, is with the scenario where only 1 participants is in the pool, which realistically would never happen.

However, if you were able to demonstrate that C can break with a PoC given the environment +/- related to the simulation we shared, we'd reward this issue a high.

[Bauchibred]

Hi @0xRektora, appreciate you forwarding the collab playground, spent a few hours using the function implementation of releaseTap() & participate() while trying to tweak the series of actions in the simulation to try to break the solvency of C, which also made me dive deeper and understand the bug case more, just as in your original simulation only the third iteration had an aM value > C from my simulations asides the first, so from my research, I concur that the likelihood of this is very low when we consider the chances of there being only one participant in the epoch.

However I think this doesn't affect the validity of the report, but the severity, since if we have a high Impact situation (An attacker monopolizing the gov), with a very low likelihood (only happens when the attacker is the first and only participant), this normally finalizes as medium severity per my experience in the web3 security space.

[maarcweiss]

Hey Bauchired, thanks for participating! The scenario: with a very low likelihood (only happens when the attacker is the first and only participant), is not very low likelihood, it will never ever happen.

Nevertheless, we will mark it as low and you will receive a reward

[maarcweiss]

Hi! We said we were not going to reward lows, but we are going to reward you with 150 USDC as a token of appreciation