sherlock-audit / 2023-11-convergence-judging

8 stars 8 forks source link

detectiveking - SdtStakingPositionManager NFT doesn't claim rewards before burning #191

Closed sherlock-admin closed 10 months ago

sherlock-admin commented 10 months ago

detectiveking

medium

SdtStakingPositionManager NFT doesn't claim rewards before burning

Summary

SdtStakingPositionManager NFT doesn't claim rewards before burning, which could lead to loss of user rewards

Vulnerability Detail

In SdtStakingPositionManager, this is what the burn function looks like:

    function burn(uint256 _tokenId) external onlyNftOwner(_tokenId) {
        require(
            ISdtStakingPositionService(stakingPerTokenId[_tokenId]).tokenTotalStaked(_tokenId) == 0,
            "TOTAL_STAKED_NOT_EMPTY"
        );
        _burn(_tokenId);
    }

The function is clearly designed to prevent users from losing funds, since it checks that the total staked amount for the tokenId is 0. However, even if the total staked amount is 0, there could be rewards left to claim (e.g. user deposited, withdrew later, but hasn't claimed rewards yet). The burn function does not check this and also does not claim them on the users behalf.

Furthermore, after the NFT is burned, the user will be unable to claim rewards with that tokenId, since claimCvgSdtRewards in StdStakingPositionService.sol for example calls checkCompliance(_tokenId) which requires the tokenId to exist.

Impact

User rewards could be trapped because the NFT is burned before rewards are claimed

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Staking/StakeDAO/SdtStakingPositionManager.sol#L152-L158

Tool used

Manual Review

Recommendation

Either call claim rewards before nft is burned or prevent user from burning nft if there are rewards to be claimed

Duplicate of #115