sherlock-audit / 2023-04-jojo-judging

7 stars 4 forks source link

yixxas - Users can incidentally DOSed themselves if too many positions are opened #390

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

yixxas

high

Users can incidentally DOSed themselves if too many positions are opened

Summary

A user that has opened too many position will no longer be able to _realizePnL.

Vulnerability Detail

Each new position opened adds to the size of the array openPositions[].

function _openPosition(Types.State storage state, address trader) internal {
    state.openPositions[trader].push(msg.sender);
}

This array is looped through in _realizePnL. Due to the maximum block gas limit of the evm, if the size of the array is too large, user will not be able to _realizePnL, and hence not able to settle any of their positions.

function _realizePnl(
    Types.State storage state,
    address trader,
    int256 pnl
) internal {
    state.primaryCredit[trader] += pnl;
    state.positionSerialNum[trader][msg.sender] += 1;

    address[] storage positionList = state.openPositions[trader];
    for (uint256 i = 0; i < positionList.length;) {
        if (positionList[i] == msg.sender) {
            positionList[i] = positionList[positionList.length - 1];
            positionList.pop();
            break;
        }
        unchecked {
            ++i;
        }
    }
}

Furthermore, the only way in which positions can be trimmed is that their profits have been realized. There is no other way to remove a position. Hence, when a user reaches this state, their positions will be stucked.

Impact

Assets cannot be settled, causing funds to be lost

Code Snippet

https://github.com/sherlock-audit/2023-04-jojo/blob/main/smart-contract-EVM/contracts/lib/Position.sol#L11-L42

Tool used

Manual Review

Recommendation

Consider adding a maximum number of positions that user can open, to prevent this state from happening.

Duplicate of #47