ak1 - StrategyLib.sol#L112 : `_loadContext` is not accumulating the market's `maintenance`

[sherlock-admin]

ak1

high

StrategyLib.sol#L112 : _loadContext is not accumulating the market's maintenance

Summary

when loading the market context, the parameters related to market context are updated. For each positions there will be a maintenance. But when loading the maintenance for each position, the values are not accumulated, instead, the last maintenance value is updated as maintenance from the function _loadContext

The function _loadContext is called when calculating the allocation value for each market inside the function allocate

Vulnerability Detail

Lets look at the function _loadContext,

    function _loadContext(Registration memory registration) private view returns (MarketContext memory context) {
        context.marketParameter = registration.market.parameter();
        context.riskParameter = registration.market.riskParameter();
        context.local = registration.market.locals(address(this));
        context.currentAccountPosition = registration.market.pendingPositions(address(this), context.local.currentId);

        Global memory global = registration.market.global();

        context.latestPrice = global.latestPrice.abs();
        context.currentPosition = registration.market.pendingPosition(global.currentId);

        for (uint256 id = context.local.latestId; id < context.local.currentId; id++)
            context.maintenance = registration.market.pendingPositions(address(this), id) -------------->>> not accumulated here.
                .maintenance(OracleVersion(0, global.latestPrice, true), context.riskParameter)
                .max(context.maintenance);
    }

lets check where this _loadContext is called, in allocate

    function allocate(
        Registration[] memory registrations,
        UFixed6 collateral,
        UFixed6 assets
    ) internal view returns (MarketTarget[] memory targets) {
        MarketContext[] memory contexts = new MarketContext[](registrations.length);
        for (uint256 marketId; marketId < registrations.length; marketId++)
            contexts[marketId] = _loadContext(registrations[marketId]); ------------------------>>> called here.

        (uint256 totalWeight, UFixed6 totalMaintenance) = _aggregate(registrations, contexts);
.
.
.

One of the place where the allocate function is used is, inside the vault contract when managing the the internal collateral and position strategy of the vault. Refer the line

Impact

Loss of maintanance. For single market it seems be small, but when it is accumulated over n number of markets, the loss would be big.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/lib/StrategyLib.sol#L59-L68

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/lib/StrategyLib.sol#L100-L115

Tool used

Manual Review

Recommendation

Update the function _loadContext as shown below.

    function _loadContext(Registration memory registration) private view returns (MarketContext memory context) {
        context.marketParameter = registration.market.parameter();
        context.riskParameter = registration.market.riskParameter();
        context.local = registration.market.locals(address(this));
        context.currentAccountPosition = registration.market.pendingPositions(address(this), context.local.currentId);

        Global memory global = registration.market.global();

        context.latestPrice = global.latestPrice.abs();
        context.currentPosition = registration.market.pendingPosition(global.currentId);

        for (uint256 id = context.local.latestId; id < context.local.currentId; id++)
            context.maintenance = registration.market.pendingPositions(address(this), id) ------>>> ------
            context.maintenance += registration.market.pendingPositions(address(this), id)----->>> ++++
                .maintenance(OracleVersion(0, global.latestPrice, true), context.riskParameter)
                .max(context.maintenance);
    }

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

141345 commented:

h

panprog commented:

invalid because maintenance doesn't need to be summed, a max of all maintenance is chosen because these are different snapshots and the max from all snapshots is chosen

[arjun-io]

Panprog is correct - maintenance does not need to be summed (maybe the Watson may be confusing it with collateral?) - instead, in order to not get liquidated, the vault needs to check it's max maintenance across pending positions and ensure that maintenance requirement is met.

[aktech297]

Escalate

Maintenance should be applied across all the pending positions. In current implementation, only the max value from all the pending positions is considered which is not correct. i.e single value is considered.

So, its valid issue.

[sherlock-admin2]

Escalate

Maintenance should be applied across all the pending positions. In current implementation, only the max value from all the pending positions is considered which is not correct. i.e single value is considered.

So, its valid issue.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[panprog]

Escalate

This issue is invalid, correct calculation is Max, not Sum, because the calculation of pending positions in question concerns change of position in time, not separate positions.

Here is an example to demonstrate why Max instead of Sum should be used.

1. Current vault position is ETH long = 10 (latest=1, current = 1, pending[1] = long 10)
2. Vault requests to increase position to long = 100 (latest = 1, current = 2, pending[2] = long 100)
3. Vault requests to increase position to long = 110 (latest = 1, current = 3, pending[3] = long 110)
4. Vault requests to decrease position to long = 10 (latest = 1, current = 4, pending[4] = long 10)

This means that Vault pending position changes are: 10 -> 100 -> 110 -> 10. It doesn't mean that vault has all 4 positions opened at the same time. What we want to know is the maintenance requirement to keep vault healthy at all times. This is obviously the maximum maintenance from all pending positions.

If maintanence = 1% and ETH=$100, then max(maintenance) = 110 $100 1% = $110 - this means that the vault minimum maintenance to keep position healthy is $110, which is correct (any value less than $110 will make position liquidatable once vault position = 110 settles).

If we calculate sum instead, we'll get (10+100+110) $100 1% = $220 - this is double of minimum maintenance required and doesn't make sense.

[sherlock-admin2]

Escalate

This issue is invalid, correct calculation is Max, not Sum, because the calculation of pending positions in question concerns change of position in time, not separate positions.

Here is an example to demonstrate why Max instead of Sum should be used.

1. Current vault position is ETH long = 10 (latest=1, current = 1, pending[1] = long 10)
2. Vault requests to increase position to long = 100 (latest = 1, current = 2, pending[2] = long 100)
3. Vault requests to increase position to long = 110 (latest = 1, current = 3, pending[3] = long 110)
4. Vault requests to decrease position to long = 10 (latest = 1, current = 4, pending[4] = long 10)

This means that Vault pending position changes are: 10 -> 100 -> 110 -> 10. It doesn't mean that vault has all 4 positions opened at the same time. What we want to know is the maintenance requirement to keep vault healthy at all times. This is obviously the maximum maintenance from all pending positions.

If maintanence = 1% and ETH=$100, then max(maintenance) = 110 $100 1% = $110 - this means that the vault minimum maintenance to keep position healthy is $110, which is correct (any value less than $110 will make position liquidatable once vault position = 110 settles).

If we calculate sum instead, we'll get (10+100+110) $100 1% = $220 - this is double of minimum maintenance required and doesn't make sense.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[hrishibhat]

Result: Invalid Unique This is a non issue based on the Sponsor comment and Escalation

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• aktech297: rejected