merlinboii - Incorrect index handling in checkpoint creation leads to incorrect initial checkpoint retrieval and potential DoS

[sherlock-admin3]

merlinboii

Medium

Incorrect index handling in checkpoint creation leads to incorrect initial checkpoint retrieval and potential DoS

Summary

In the current implementation of ProtectedListings::_createCheckpoint(), when multiple listings are created for the same collection at the same timestamp, the existing checkpoint is updated, and no new checkpoint is pushed.

However, the function incorrectly returns the wrong index for this case leads to incorrect index referencing during subsequent listing creations.

Vulnerability Detail

When a checkpoint is created at the same timestamp, the existing checkpoint is updated, and no new checkpoint is pushed.

ProtectedListings::_createCheckpoint()

File: ProtectedListings.sol
530:     function _createCheckpoint(address _collection) internal returns (uint index_) {
531:         // Determine the index that will be created
532:         index_ = collectionCheckpoints[_collection].length;
---
559:         // Get our new (current) checkpoint
560:         Checkpoint memory checkpoint = _currentCheckpoint(_collection);
561: 
562:         // If no time has passed in our new checkpoint, then we just need to update the
563:         // utilization rate of the existing checkpoint.
564:@>         if (checkpoint.timestamp == collectionCheckpoints[_collection][index_ - 1].timestamp) {
565:@>             collectionCheckpoints[_collection][index_ - 1].compoundedFactor = checkpoint.compoundedFactor;
566:@>             return index_;
567:         }
---
571:     }

However, the current implementation returns the wrong index for this case, causing incorrect checkpoint handling for new listing creations, especially when creating multiple listings for the same collection with different variations.

ProtectedListings::createListings()

File: ProtectedListings.sol
116:      */
117:     function createListings(CreateListing[] calldata _createListings) public nonReentrant lockerNotPaused {
---
134:             checkpointKey = keccak256(abi.encodePacked('checkpointIndex', listing.collection));
135:             assembly { checkpointIndex := tload(checkpointKey) }
136:             if (checkpointIndex == 0) {
137:                 checkpointIndex = _createCheckpoint(listing.collection);
138:                 assembly { tstore(checkpointKey, checkpointIndex) }
139:             }
---
143:             tokensReceived = _mapListings(listing, tokensIdsLength, checkpointIndex) * 10 ** locker.collectionToken(listing.collection).denomination();
---
156:     }

An edge case arises when a new listing is created for a collection that has no checkpoints (collectionCheckpoints[_collection].length == 0).

Assuming erc721b has no existing checkpoints (length = 0):

• Creating 2 CreateListings for the same collection (erc721b) with different variants should result in only one checkpoint being created.
• In the first iteration, the _createCheckpoint() returns 0 as the index, stores it in checkpointIndex, and updates the transient storage at the checkpointKey slot. The listing is then stored with the current checkpoint.

ProtectedListings::createListings()

File: ProtectedListings.sol
116:      */
117:     function createListings(CreateListing[] calldata _createListings) public nonReentrant lockerNotPaused {
---
134:             checkpointKey = keccak256(abi.encodePacked('checkpointIndex', listing.collection));
135:             assembly { checkpointIndex := tload(checkpointKey) }
136:@>           if (checkpointIndex == 0) {
137:@>               checkpointIndex = _createCheckpoint(listing.collection);
138:@>               assembly { tstore(checkpointKey, checkpointIndex) }
139:             }
---
143:             tokensReceived = _mapListings(listing, tokensIdsLength, checkpointIndex) * 10 ** locker.collectionToken(listing.collection).denomination();
---
156:     }

• In the second iteration, since checkpointKey stores 0, _createCheckpoint() is triggered again and returns 1 (the length of checkpoints) even though no new checkpoint was pushed.

As a result, the second iteration incorrectly references index 1, even though the checkpoint only exists at index 0 (with a length of 1). This causes incorrect indexing for the listings.

Impact

Incorrect index returns lead to the wrong initial checkpoint index for new listings, causing incorrect checkpoint retrieval and utilization. This can result in inaccurate data and potential out-of-bound array access, leading to a Denial of Service (DoS) in ProtectedListings.unlockPrice()

Code Snippet

ProtectedListings::_createCheckpoint()

File: ProtectedListings.sol
530:     function _createCheckpoint(address _collection) internal returns (uint index_) {
531:         // Determine the index that will be created
532:         index_ = collectionCheckpoints[_collection].length;
---
559:         // Get our new (current) checkpoint
560:         Checkpoint memory checkpoint = _currentCheckpoint(_collection);
561: 
562:         // If no time has passed in our new checkpoint, then we just need to update the
563:         // utilization rate of the existing checkpoint.
564:@>         if (checkpoint.timestamp == collectionCheckpoints[_collection][index_ - 1].timestamp) {
565:@>             collectionCheckpoints[_collection][index_ - 1].compoundedFactor = checkpoint.compoundedFactor;
566:@>             return index_;
567:         }
---
571:     }

ProtectedListings::createListings()

File: ProtectedListings.sol
116:      */
117:     function createListings(CreateListing[] calldata _createListings) public nonReentrant lockerNotPaused {
---
134:             checkpointKey = keccak256(abi.encodePacked('checkpointIndex', listing.collection));
135:             assembly { checkpointIndex := tload(checkpointKey) }
136:@>           if (checkpointIndex == 0) {
137:@>               checkpointIndex = _createCheckpoint(listing.collection);
138:@>               assembly { tstore(checkpointKey, checkpointIndex) }
139:             }
---
143:             tokensReceived = _mapListings(listing, tokensIdsLength, checkpointIndex) * 10 ** locker.collectionToken(listing.collection).denomination();
---
156:     }

ProtectedListings::unlockPrice()

File: ProtectedListings.sol
607:     function unlockPrice(address _collection, uint _tokenId) public view returns (uint unlockPrice_) {
608:         // Get the information relating to the protected listing
609:         ProtectedListing memory listing = _protectedListings[_collection][_tokenId];
610: 
611:         // Calculate the final amount using the compounded factors and principle amount
612:         unlockPrice_ = locker.taxCalculator().compound({
613:             _principle: listing.tokenTaken,
614:             _initialCheckpoint: collectionCheckpoints[_collection][listing.checkpoint],
615:             _currentCheckpoint: _currentCheckpoint(_collection)
616:         });
617:     }

Tool used

Manual Review

Recommendation

Update the return value of the ProtectedListings::_createCheckpoint() to return index_ - 1 when the checkpoint is updated at the same timestamp to ensure that subsequent listings reference the correct index.

function _createCheckpoint(address _collection) internal returns (uint index_) {
    // Determine the index that will be created
    index_ = collectionCheckpoints[_collection].length;
---
    // If no time has passed in our new checkpoint, then we just need to update the
    // utilization rate of the existing checkpoint.
    if (checkpoint.timestamp == collectionCheckpoints[_collection][index_ - 1].timestamp) {
        collectionCheckpoints[_collection][index_ - 1].compoundedFactor = checkpoint.compoundedFactor;
-        return index_;
+        return (index_ - 1);
    }
---
}