settle(address(0)) global overwritten by local

Summary

when execute Vault.settle(address(0)) ,since account=address(0), context.local actually takes the value of global, but performs the logical calculation of local, and then overwrites global with local at the end, which results in the misaligned value of global.

Vulnerability Detail

Anyone can call Vault.settle(address(0)) to settle global The register() updateMarket() updateParameter methods also call settle(address(0)) internally

The code implementation is as follows.

    function settle(address account) public whenNotPaused {
        _settleUnderlying();
@>      Context memory context = _loadContext(account);

@>      _settle(context);
        _manage(context, UFixed6Lib.ZERO, false);
@>      _saveContext(context, account);
    }

The first step is to call the _loadContext(account)

    function _loadContext(address account) private view returns (Context memory context) {
....

        context.global = _accounts[address(0)].read();
@>      context.local = _accounts[account].read();
        context.latestCheckpoint = _checkpoints[context.global.latest].read();
    }

Since account=address(0), context.local is getting the same value as global , both are _accounts[address(0)]

The second step calls _settle(), which calculates both global and local, and since local is currently taken from global it is the wrong

    function _settle(Context memory context) private {
        // settle global positions
        while (
            context.global.current > context.global.latest &&
            _mappings[context.global.latest + 1].read().ready(context.latestIds)
        ) {
            uint256 newLatestId = context.global.latest + 1;
            context.latestCheckpoint = _checkpoints[newLatestId].read();
            (Fixed6 collateralAtId, UFixed6 feeAtId, UFixed6 keeperAtId) = _collateralAtId(context, newLatestId);
            context.latestCheckpoint.complete(collateralAtId, feeAtId, keeperAtId);

@>          context.global.processGlobal(
                newLatestId,
                context.latestCheckpoint,
                context.latestCheckpoint.deposit,
                context.latestCheckpoint.redemption
            );
            _checkpoints[newLatestId].store(context.latestCheckpoint);
        }

        // settle local position
        if (
            context.local.current > context.local.latest &&
            _mappings[context.local.current].read().ready(context.latestIds)
        ) {
            uint256 newLatestId = context.local.current;
            Checkpoint memory checkpoint = _checkpoints[newLatestId].read();
@>          context.local.processLocal(
                newLatestId,
                checkpoint,
                context.local.deposit,
                context.local.redemption
            );
        }
    }

The third step calls _saveContext(), and the wrong local calculated in the second step will be used as global, overriding the real global.

    function _saveContext(Context memory context, address account) private {
        _accounts[address(0)].store(context.global);
@>      _accounts[account].store(context.local);
        _checkpoints[context.currentId].store(context.currentCheckpoint);
    }

Since account is equal to address(0), local overrides global.

This results in an incorrect value in global, and an incorrect latestCheckpoint calculated in the second _settle(context) step.

Impact

global values were incorrectly calculated and overwritten, resulting in multiple dependencies on global being calculated incorrectly

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L190-L197

Tool used

Manual Review

Recommendation

_settle(context) and _saveContext() Ignoring the handling of account=address(0)

    function _saveContext(Context memory context, address account) private {
        _accounts[address(0)].store(context.global);
-       _accounts[account].store(context.local);
+       if(account!=address(0)) _accounts[account].store(context.local);
        _checkpoints[context.currentId].store(context.currentCheckpoint);
    }

Duplicate of #62

sherlock-audit / 2023-07-perennial-judging

bin2chen - settle(address(0)) global overwritten by local #147