Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User can avoid force exercise by front-running and mint/burn another position #532

Closed
c4-bot-1 opened this issue Apr 22, 2024 · 2 comments
Closed
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-352 grade-b QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_352_group AI based duplicate group recommendation

Comments

@c4-bot-1
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1268
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L893
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L1367-#L1395

Vulnerability details

Vulnerability details

When calling forceExercise() function, _validateSolvency() is called to check positionIdList of account:

     _validateSolvency(account, positionIdListExercisee, NO_BUFFER);  // <---

It will validate position list of account:

function _validateSolvency(
    address user,
    TokenId[] calldata positionIdList,
    uint256 buffer
) internal view returns (uint256 medianData) {
    // check that the provided positionIdList matches the positions in memory
    _validatePositionList(user, positionIdList, 0);   // <---

This function will revert if hash is incorrect:

function _validatePositionList(
    address account,
    TokenId[] calldata positionIdList,
    uint256 offset
) internal view {
    uint256 pLength;
    uint256 currentHash = s_positionsHash[account];  // <---

    unchecked {
        pLength = positionIdList.length - offset;
    }
    // note that if pLength == 0 even if a user has existing position(s) the below will fail b/c the fingerprints will mismatch
    // Check that position hash (the fingerprint of option positions) matches the one stored for the '_account'
    uint256 fingerprintIncomingList;

    for (uint256 i = 0; i < pLength; ) {
        fingerprintIncomingList = PanopticMath.updatePositionsHash(
            fingerprintIncomingList,
            positionIdList[i],
            ADD
        );
        unchecked {
            ++i;
        }
    }

    // revert if fingerprint for provided '_positionIdList' does not match the one stored for the '_account'
    if (fingerprintIncomingList != currentHash) revert Errors.InputListFail();  // <---

And the hash is updated whenever nwe position of user is updated/removed by calling _updatePositionsHash() function:

function _updatePositionsHash(address account, TokenId tokenId, bool addFlag) internal {
    uint256 newHash = PanopticMath.updatePositionsHash(
        s_positionsHash[account],
        tokenId,
        addFlag
    );
    if ((newHash >> 248) > MAX_POSITIONS) revert Errors.TooManyPositionsOpen();
    s_positionsHash[account] = newHash;   // <---
}

It lead to scenario that account can avoid being forced to execute by minting/burning other option that to make sure numbers of option of the user is is smaller than MAX_POSITIONS

Impact

Option can be avoid to be force executed, user's funds is locked.

Tools Used

Manual review

Recommended Mitigation Steps

Mechanism related to hash of position list should be changed.

Assessed type

Context

@c4-bot-1 c4-bot-1 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 22, 2024
c4-bot-2 added a commit that referenced this issue Apr 22, 2024
@c4-bot-11 c4-bot-11 added the 🤖_352_group AI based duplicate group recommendation label Apr 22, 2024
@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #352

@c4-judge c4-judge removed the 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value label May 6, 2024
@c4-judge
Copy link
Contributor

c4-judge commented May 6, 2024

Picodes changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-352 grade-b QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_352_group AI based duplicate group recommendation
Projects
None yet
Development

No branches or pull requests

4 participants