code-423n4 / 2024-06-panoptic-findings

1 stars 0 forks source link

Issue M-02 not correctly fixed since the check is not inclusive #25

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1371-L1384

Vulnerability details

Proof of Concept

See M-02 _here_

TLDR of the report is that the _validatePositionList() function fails to detect duplicate token IDs. And this oversight allows attackers to bypass solvency checks, enabling insolvent users to perform actions like minting, burning, liquidating, and force exercising options, as well as settling long premiums on behalf of other insolvent users.

Now, the bug stems from the function's inability to differentiate between unique and duplicated token IDs, which can be exploited by adding multiple instances of the same token ID to generate a duplicate position hash. This manipulation can inflate an account's collateral balance beyond its required level, falsely indicating solvency.

So to mitigate this vulnerability, wardens recommended to introduce a check in _validatePositionList() to ensure the token ID array length is shorter than MAX_POSITIONS (32)., however going to the implemented fix, this has not been accurately done, as the check is inclusive and actually erroneously accepts the data passed in even if the array length is not shorter than MAX_POSITIONS, see https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L1371-L1384

    function _updatePositionsHash(address account, TokenId tokenId, bool addFlag) internal {
        // Get the current position hash value (fingerprint of all pre-existing positions created by '_account')
        // Add the current tokenId to the positionsHash as XOR'd
        // since 0 ^ x = x, no problem on first mint
        // Store values back into the user option details with the updated hash (leaves the other parameters unchanged)
        uint256 newHash = PanopticMath.updatePositionsHash(
            s_positionsHash[account],
            tokenId,
            addFlag
        );
        if ((newHash >> 248) > MAX_POSITIONS) revert Errors.TooManyPositionsOpen();
        s_positionsHash[account] = newHash;
    }

Which would mean that when (newHash >> 248) == MAX_POSITIONS the execution does not revert where as it should.

Impact

M-02 has not been accurately fixed, leaving protocol susceptible to all the attacks mentioned in the original submission.

Recommended Mitigation Steps

Consider applying the fix as was in the original submission, i.e :


-        if ((newHash >> 248) > MAX_POSITIONS) revert Errors.TooManyPositionsOpen();
+        if ((newHash >> 248) >= MAX_POSITIONS) revert Errors.TooManyPositionsOpen();
        s_positionsHash[account] = newHash;
    }

Assessed type

Context

c4-judge commented 1 month ago

Picodes marked the issue as unsatisfactory: Invalid