Stake Position NFT owner can always delay the claim or withdraw by max 10 days right before the sale of token NFT.

[hats-bug-reporter[bot]]

Github username: @https://github.com/bluenights004 Twitter username: @bluenights004 Submission hash (on-chain): 0x98c3b1cd14cdb8373677cd5c0a708b7c1396fed91641f58f30feecdc40beff9d Severity: medium

Description: Description\ Staking positions are tokenized through minting NFT and these NFTs can be subject for trading in open market. Each NFT has a lock function to forbid the owner to either withdraw funds or claim rewards during a given duration. This function has been implemented so NFT buyers can’t be frontruned and receive an empty NFT.

However, the malicious current NFT owner still can do some griefing or annoyance to the NFT buyer through the setLock function (see below). This can be set to a maximum of 10 days lock right before the sale (through frontrunning) in which the future owner (buyer) has no power to revise or edit. The future owner has no choice but to wait another 10 days to claim the rewards or withdraw the stake position.

These supposed to be claimed rewards or withdrawed stake assets can be use immediately somewhere else in which the buyer might gain or profit from, so the effect of griefing can bring some kind of opportunity cost to the part of buyer. Lost opportunity due to time delayed in claiming rewards or withdrawal of assets.

File: CvgERC721TimeLockingUpgradeable.sol
57:     /// @notice As the Token Owner, set a timelock until a timestamp
58:     /// @param tokenId token to timelock
59:     /// @param timestamp timestamp where the timelock ends
60:     function setLock(uint256 tokenId, uint256 timestamp) external onlyNftOwner(tokenId) {
61:         require(timestamp >= block.timestamp + BUFFER, "TIME_BUFFER");
62:         require(timestamp - block.timestamp < maxLockingTime, "MAX_TIME_LOCK");
63:         require(timestamp > unlockingTimestampPerToken[tokenId], "ALREADY_LOCKED");
64: 
65:         unlockingTimestampPerToken[tokenId] = timestamp;
66:     }
67: }
68: 

Attack Scenario\ Here is possible exploit scenario:

1. User A, the current NFT owner decided to sell his tokenized stake position to the open market. Let's say the market value offered by the seller User A is worth 1,000 USDC (this includes the value of earned rewards).
2. User B, interested buyer see this NFT in the marketplace and want to purchase this immediately. He intended to use the assets and rewards inside the NFT to be exchanged and invested somewhere else in which he can gain more interest and benefits.
3. User A, with a malicious intent in the beginning, see User B's transaction trying to buy his NFT, decided to frontrun it by setting the lock to the maximum of 10 days.
4. User B, eager to withdraw and claim the rewards of the recently purchased NFT tokenized position, has failed to do his intention. He just realized, timelock has been revised right before sale.
5. After learning what happened, User B has no choice but to wait and lost the opportunity to earn more in some platforms with higher rewards and benefits due to time delay.

Attachments

Proof of Concept

To demonstrate the vulnerability, here's the process flow affected.

1. These functions Claiming rewards and Withdraw stake have modifier checkCompliance. This modifier should be passed in order to execute these functions.

   File: StakingServiceBase.sol
   224:     modifier checkCompliance(uint256 tokenId) {
   225:         stakingPositionManager.checkTokenFullCompliance(tokenId, msg.sender);
   226:         _;
   227:     }
   228: 

   File: CvxStakingPositionManager.sol
   76:     function checkTokenFullCompliance(uint256 tokenId, address receiver) external view {
   77:         /// @dev Verify that receiver is always the NFT owner
   78:         require(receiver == ownerOf(tokenId), "TOKEN_NOT_OWNED");
   79:         /// @dev As the StakingPositionManager is the NFT contract, we verify that this ID has been created from this StakingPositionService
   80:         require(msg.sender == stakingPerTokenId[tokenId], "WRONG_STAKING");
   81:         /// @dev We verify that the tokenId is not timelocked
   82:         require(unlockingTimestampPerToken[tokenId] < block.timestamp, "TOKEN_TIMELOCKED");
   83:     }

2. In function checkTokenFullCompliance (see above), you can see the line 82 in which it requires the current blocktimestamp to be more than the unlockingTimestamp of the token ID. If this can't be met, the user will be unable to withdraw assets or claim rewards.

3. This part of the code (line 82) can be griefed by the NFT owner if he intend to do so through this function setLock (see below). The maximum days he can delay is 10 days, based on the contract setup.

   function setLock(uint256 tokenId, uint256 timestamp) external onlyNftOwner(tokenId) {
       require(timestamp >= block.timestamp + BUFFER, "TIME_BUFFER");
       require(timestamp - block.timestamp < maxLockingTime, "MAX_TIME_LOCK");
       require(timestamp > unlockingTimestampPerToken[tokenId], "ALREADY_LOCKED");
   
       unlockingTimestampPerToken[tokenId] = timestamp;
   }
   }

Recommendation

The NFT tokenized position owner should not be able to make changes to the NFT details right before sale. There should be some restriction in place once it is intended to be sold in the open market.

[PlamenTSV]

The locking NFT mechanism is used to ensure the NFT buyer that the seller would not be able to front-run the transfer and claim/withdraw all funds from the NFT. TL;DR this is intended as it is a fraud protection mechanism. The 10 days buffer is intended.

[bluenights004]

Hello PlamenTSV, The 10 days here does not refer to buffer, it refers to the locking period. The locking period in which the future buyer or holder of NFT can't do any reward claims or withdraw of assets. This can be categorized as temporary freeze of assets, in which the protocol never intend or design to do but somehow manipulated by malicious nft owner to grief the future buyer.

Hope you can take a look again. Appreciate thanks.

[PlamenTSV]

There is no funds lost, they are locked intentionally. Your step-by-step PoC scenario is flawed. Instead of locking the NFT, user A can instead withdraw all funds from it and send user B a useless NFT, which is why the contract enforces a lock on the seller. Sponsor when I asked him for the purpose of the lock: "The NFT position this unlockingTimestamp is for user puting their NFT in sell on a platform such as open sea It allows buyers to don't get front run" The protocol puts the upper limit to 10 days and freely allows the locking, so I don't know how you assumed that the protocol didn't design or intend this. If you find a way that a user can lock his NFT for more than the said 10 days, then this can be valid.

[bluenights004]

Appreciate your response, PlamenTSV

Just need to clarify that what I'm telling you about the protocol never intended to design or intent to do is about "for future buyer is unable to do their claims or withdraw their assets in a certain period". I'm referring to the "future buyer" victim of frontrunning by the seller. This will never be intention by the protocol design. Let's focus on that.

The protocol design and intention is all well in this part.

Protocol design -> (the seller won't be able to withdraw any assets from nft before the sale), i agree with that,

but let's focus on this issue on what my submission is all about

My Submitted Issue -> the seller is still capable of griefing (thru frontrunning) the future buyer by delaying his claims and withdrawal once nft is transferred to him (buyer). There is no way for future buyer to revise the locking period even if he already owned the nft.

By the way, need to also add detail on my recommended mitigation in setlock function to resolve the issue. In this way, the current NFT owner can't revise the timelock during lock period in which the NFT is currently on sale. This can prevent griefing by seller to the future buyer, making sure the sale is complete with no sudden revision of timelock. Please see below the revised code:

function setLock(uint256 tokenId, uint256 timestamp) external onlyNftOwner(tokenId) {
        require(timestamp >= block.timestamp + BUFFER, "TIME_BUFFER");
        require(timestamp - block.timestamp < maxLockingTime, "MAX_TIME_LOCK");
        require(timestamp > unlockingTimestampPerToken[tokenId], "ALREADY_LOCKED");
  ++ require(block.timestamp > unlockingTimestampPerToken[tokenId], "CAN'T REVISE TIMELOCK");

        unlockingTimestampPerToken[tokenId] = timestamp;
    }
}