Protocol's internal accounting would be broken whenever a singularity gets re-added

[hats-bug-reporter[bot]]

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

Description: Description

Subreport 1

See link.

The idea of the faulting of the accounting has been sufficiently explained in the linked report.

However, no fix was applied for this, considering navigating to https://github.com/hats-finance/Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad/blob/baddafc15616a5674e73c2f5a920b92bf9b21888/contracts/options/TapiocaOptionBroker.sol#L376-L398 shows that no update is made to the accounting when the singularity is in recovery rescue, i.e

        // If the SGL is in rescue mode, bypass the voting power removal
|>        if (!isSGLInRescueMode && participation.hasVotingPower) {
            TWAMLPool memory pool = twAML[lock.sglAssetID];

            if (participation.divergenceForce) {
                if (pool.cumulative > participation.averageMagnitude) {
                    pool.cumulative -= participation.averageMagnitude;
                } else {
                    pool.cumulative = EPOCH_DURATION;
                }
            } else {
                pool.cumulative += participation.averageMagnitude;
            }

            pool.totalDeposited -= lock.ybShares;

            unchecked {
                --pool.totalParticipants;
            }

            twAML[lock.sglAssetID] = pool; // Save twAML exit
            emit AMLDivergence(epoch, pool.cumulative, pool.averageMagnitude, pool.totalParticipants); // Register new voting power event
        }

Attack Scenario As explained in the linked report, in certain situations, Singularities can be unregistered. In such cases, they're first set to rescue mode from TapiocaOptionLiquidityProvision. Now, within the TapiocaOptionBroker contract, if a user tries to exit their position, when the Singularity is in rescue mode, the twAML accounting is not adjusted, including cumulative, totalDeposited, and totalParticipants.

This then leads to a case where if the singularity is re-added, all twAML accounting henceforth will be incorrect.

Subreport 2

See https://github.com/code-423n4/2024-02-tapioca-findings/issues/65

A similar logic to Subreport1 in the sense that the bug case is rooted around how re-adding singularities would lead to the wrong value for netDepositedForEpoch being set. Logic is similar to subreport1, though in this case, after the singularity is removed, users are then allowed to unlock early, and these early unlocks would not adjust the netDepositedForEpoch value. This then means that re-adding the singularity again would cause an inflated value within netDepositedForEpoch, which would make a part of TAP options allocated towards said singularity unclaimable.

Essentially causing the invariant below to be broken for twAML:

The sum of all balances at a given epoch is the sum of all active locks.

Recommendation Apply the fix as suggested here, which is to apply accounting updates even if the singularity is in rescue mode.

For Subreport 2, consider adjusting netDepositedForEpoch when unlocks are being done early. Attachments

1. Proof of Concept (PoC) File

See linked reports 1 & 2

2. Revised Code File (Optional)

N/A

[0xRektora]

We might have missed the C4 issue as it wasn't validated by us. But we do not intend to put back a compromised/deprecated market.

[Bauchibred]

Hi @0xRektora, thanks for the feedback, contrary to what you've commented though, I assume this bug case is valid, considering @maarcweiss confirmed the group of findings around this bug window from the previous audit. See confirmation here, or is there something else I'm missing?

[0xRektora]

We're firm with the judgement. Just take the perspective of the protocol. Will you re add a compromised/bugged market?

[Bauchibred]

Agree I didn't take the compromising factor into consideration, so in your case only way it could be re-added is if the compromisation is not permanent (or never as is currently done to ensure protocol safety)., or if it was not removed in the first place due to a compromisation/bug and just deprecated.

The instance from the tolp contract for when singularities are to be removed didn't explicitly specify that it would always be due to a compromisation or bug case which is why I concluded this should be in scope.