search

sherlock-audit / 2023-11-convergence-judging

8 stars 8 forks source link

0xmuxyz - The locked-amount and the voting power can still be increased even after the given `tokenId` of locking position NFT (ERC721) would be burned via the LockingPositionService#`burnPosition()` #210

Closed sherlock-admin2 closed 11 months ago

sherlock-admin2 commented 11 months ago

0xmuxyz

medium

The locked-amount and the voting power can still be increased even after the given tokenId of locking position NFT (ERC721) would be burned via the LockingPositionService#burnPosition()

Summary

Due to lack of logic to delete the record of the locking position from the lockingPositions storage, the locked-amount and the voting power can still be increased even after the given tokenId of locking position NFT (ERC721) would be burned via the LockingPositionService#burnPosition()

Vulnerability Detail

When a new locking position would be minted, the LockingPositionService#mintPosition() would be called. Within the mintPosition(), new tokenId would be stored into the lockingPositions storage and then the tokenId of locking position NFT (ERC721) would be minted like this: https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L293 https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L309

    /**
     * @notice Mint a locking position (ERC721) for the user.
     * @dev Lock can't be greater than the Maximum locking time / The end of the lock must finish on a TDE event cycle |  The percentage of ys determines the repartition in veCVG,mgCVG/YsCVG.
     * @param lockDuration is the duration in cycle(week) of the lock.
     * @param amount is the amount of cvg to lock in the position.
     * @param ysPercentage percentage of lock dedicated to treasury shares (ysCVG).
     * @param receiver address of the receiver of the locking position.
     * @param isAddToManagedTokens add the created token in managed tokens(voting power)  directly.
     */
    function mintPosition(
        uint96 lockDuration,
        uint256 amount,
        uint64 ysPercentage,  /// @audit info - How much is % allocated to the ysCVG. (The remaining % is going to be the veCVG / mgCVG)
        address receiver,
        bool isAddToManagedTokens
    ) external onlyWalletOrWhiteListedContract {
        ...
        LockingPosition memory lockingPosition = LockingPosition({
            startCycle: actualCycle,
            lastEndCycle: endLockCycle,
            totalCvgLocked: amount,
            mgCvgAmount: _mgCvgCreated,
            ysPercentage: ysPercentage
        });

        /** @dev Associate this Locking position on the tokenId. */
        lockingPositions[tokenId] = lockingPosition;  ///<--------------- @audit
        ...

        /** @dev Mint the ERC721 representing the user position. */
        _lockingPositionManager.mint(receiver); ///<--------------- @audit
       ...

On the other hand, when an existing locking position would be burned, the LockingPositionService#burnPosition() would be called. Within the LockingPositionService#burnPosition(), the given tokenId of locking position NFT (ERC721) would be burned like this: https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L526

    /**
     * @notice Unlock CVG tokens under the NFT Locking Position : Burn the NFT, Transfer back the CVG to the user.  Rewards from YsDistributor must be claimed before or they will be lost.    * @dev The locking time must be over
     * @param tokenId to burn
     */
    function burnPosition(uint256 tokenId) external {
        ...
        /** @dev Burn the NFT representing the position. */
        _cvgControlTower.lockingPositionManager().burn(tokenId); ///<---------- @audit

        /** @dev Transfer CVG back to the user. */
        cvg.transfer(msg.sender, totalCvgLocked);
        ...

However, within the LockingPositionService#burnPosition(), there is no logic to delete the tokenId from the lockingPositions storage. As a result, even if the given tokenId of locking position NFT (ERC721) would be burned via the LockingPositionService#burnPosition(), the record of the locking position is still remaining in the lockingPositions storage.

This is problematic. Because the locked-amount and the voting power can still be increased unless the record of the locking position would remain in the lockingPositions storage even after the given tokenId of locking position NFT (ERC721) would be burned: https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L330 https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L354 https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Locking/LockingPositionService.sol#L358

    function increaseLockAmount(
        uint256 tokenId, /// @audit info - An existing tokenId
        uint256 amount,
        address operator
    ) external onlyWalletOrWhiteListedContract {
        ...
        LockingPosition memory lockingPosition = lockingPositions[tokenId]; ///<--------- @audit
        ...

        uint256 _newVotingPower;
        /** @dev Update voting power through Curve contract, link voting power to the nft tokenId. */
        if (lockingPosition.ysPercentage != MAX_PERCENTAGE) {
            ...
            lockingPositions[tokenId].mgCvgAmount += _newVotingPower; ///<--------- @audit
        }

        /** @dev Update cvgLocked balance. */
        lockingPositions[tokenId].totalCvgLocked += amount;  ///<--------- @audit

Impact

The locked-amount and the voting power can still be increased even after the given tokenId of locking position NFT (ERC721) would be burned via the LockingPositionService#burnPosition()

Code Snippet

Tool used

Recommendation

Within the LockingPositionService#burnPosition(), consider adding a logic to delete the tokenId from the lockingPositions storage like this:

    function burnPosition(uint256 tokenId) external {
        ...
        /** @dev Burn the NFT representing the position. */
        _cvgControlTower.lockingPositionManager().burn(tokenId); 

        /** @dev Transfer CVG back to the user. */
        cvg.transfer(msg.sender, totalCvgLocked);

+       delete lockingPositions[tokenId]; 
        ...
nevillehuang commented 11 months ago

Invalid, token ownership check here would revert, since there is no longer an owner (address(0)) for the NFT representing the locking position