# The health of a ```ProtectedListing``` is incorrectly calculated if the ```tokenTaken``` has be changed through ```ProtectedListings::adjustPosition()```. Crit/High · Sherlock · NFT liquidity · 2nd September 2024 Finding H-21 of the Flayer competition. - Protocol: https://audits.sherlock.xyz/contests/468 - Codebase: https://github.com/0xsimao/2024-08-flayer/tree/0ec252cf9ef0f3470191dcf8318f6835f5ef688c - Source: https://github.com/sherlock-audit/2024-08-flayer-judging/issues/689 --- ### 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()``` : ```solidity 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](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/ProtectedListings.sol#L366C1-L417C6) 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 : ```solidity 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](https://github.com/sherlock-audit/2024-08-flayer/blob/0ec252cf9ef0f3470191dcf8318f6835f5ef688c/flayer/src/contracts/ProtectedListings.sol#L497) 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. --- Related findings: - [`TARGET_HEALTH` calculation does not consider the adjust factors of the picked seize and repay markets](https://0xsimao.com/findings/exactly-protocol-update-staking-contract-factors-seize-repay-markets): Exactly Protocol Update - Staking Contract - [Lack of deadline in PowerToken.buy can lead to user's cashToken being distributed through ZeroToken holders](https://0xsimao.com/findings/m-0-deadline-cash-through-zero): M^0 Minter Gateway - [`rewardData.releaseRate` is incorrectly calculated on `RewardsController::config()` when `block.timestamp > start` and `rewardData.lastConfig != rewardData.start`](https://0xsimao.com/findings/exactly-protocol-update-staking-contract-reward-release-rewards-timestamp): Exactly Protocol Update - Staking Contract - [stakingAssetManager in ZKToken may be immutable as it is never changed](https://0xsimao.com/findings/singularity-staking-asset-immutable-changed): Singularity