The health of a ProtectedListing is incorrectly calculated if the tokenTaken has be changed through ProtectedListings::adjustPosition().

Summary

ProtectedListings::adjustPosition() will change the tokenTaken but after this, ProtectedListings::getProtectedListingHealth() will assume that these tokenTaken has been the same as the starting ones and will incorrectly compound them.

Root Cause

A user can create a ProtectedListing by calling ProtectedListings::createPosition(), deposit his token and take back an amount tokenTake as debt. This amount is supposed to be compounded through the time depending on the newest compound factor and the compound factor when the position was created. However, when the user calls ProtectedListings::adjustPosition() and take some more debt, this new debt will be assume it will be there from the start and it will be compounded as such in ProtectedListings::getProtectedListingHealth() while this is not the case and it will be compounded from the moment it got taken. Let's have a look on ProtectedListings::adjustPosition() :

    function adjustPosition(address _collection, uint _tokenId, int _amount) public lockerNotPaused {
        // ...

        // Get the current debt of the position
@>        int debt = getProtectedListingHealth(_collection, _tokenId);

        // ...

        // Check if we are decreasing debt
        if (_amount < 0) {
            // ...

            // Update the struct to reflect the new tokenTaken, protecting from overflow
@>            _protectedListings[_collection][_tokenId].tokenTaken -= uint96(absAmount);
        }
        // Otherwise, the user is increasing their debt to take more token
        else {
           // ...

            // Update the struct to reflect the new tokenTaken, protecting from overflow
@>            _protectedListings[_collection][_tokenId].tokenTaken += uint96(absAmount);
        }

        // ...
    }

Link to code

As we can see, this function just increases or decreases the tokenTaken of this ProtectedListings meaning the debt that the owner should repay. Now, if we see the ProtectedListings::getProtectedListingHealth(), we will understand that function just takes the tokenTaken and compounds it without caring when this tokenTaken increased or decreased :

    function getProtectedListingHealth(address _collection, uint _tokenId) public view listingExists(_collection, _tokenId) returns (int) {
        // So we start at a whole token, minus: the keeper fee, the amount of tokens borrowed
        // and the amount of collateral based on the protected tax.
@>        return int(MAX_PROTECTED_TOKEN_AMOUNT) - int(unlockPrice(_collection, _tokenId));
    }

    function unlockPrice(address _collection, uint _tokenId) public view returns (uint unlockPrice_) {
        // Get the information relating to the protected listing
        ProtectedListing memory listing = _protectedListings[_collection][_tokenId];

        // Calculate the final amount using the compounded factors and principle amount
        unlockPrice_ = locker.taxCalculator().compound({
@>            _principle: listing.tokenTaken,
            _initialCheckpoint: collectionCheckpoints[_collection][listing.checkpoint],
            _currentCheckpoint: _currentCheckpoint(_collection)
        });
    }

Link to code

So, it will take this tokenTaken and it will compound it from the start of the ProtectedListing until now, while it shouldn't be the case since some debt may have been added later (through ProtectedListings::adjustPosition()) and in this way this amount must be compounded from the moment it got taken until now, not from the start.

Internal pre-conditions

1. User creates a ProtectedListing through ProtectedListings::createListings().

External pre-conditions

1. User wants take some debt on his ProtectedListing.

Attack Path

1. Firstly, user calls ProtectedListings::createListings() and creates a ProtectedListing with tokenTaken and expecting them to compound.
2. After some time and the initial tokenTaken have been compounded a bit, user decides to take some more debt and increase tokenTaken and calls ProtectedListings::adjustListing() to take x more debt.
3. Now, ProtectedListings::getProtectedListingHealth() shows that the x more debt is compounded like it was taken from the very start of the ProtectedListing creation and in this way his debt is unfairly more inflated.

Impact

The impact of this serious vulnerability is that the user is being in more debt than what he should have been, since he accrues interest for a period that he had not actually taken that debt. In this way, while he expects his tokenTaken to be increased by x amount (as it would be fairly happen), he sees his debt to be inflated by x compounded. This can cause instant and unfair liquidations and loss of funds for users unfairly taken into more debt.

PoC

No PoC needed.

Mitigation

To mitigate this vulnerability successfully, consider updating the checkpoints of the ProtectedListing whenever an adjustment is happening in the position, so the debt to be compounded correctly.