Withdrawing rewards will convert sdt to cvgSDT at any rate

Summary

In SdtStakingPositionService when SDT rewards are claimed via claimCvgSdtRewards() using _isConvert = true and _isMint = false, the sdtRewardReceiver will convert the received SDT rewards to cvgSDT using a stable pool with no slippage protection. This swap can be sandwiched to steal SDT rewards from the user.

Vulnerability Detail

SdtStakingPositionService.claimCvgSdtRewards() computes the claimable rewards and pass the array of claimable rewards to sdtRewardReceiver:

    function claimCvgSdtRewards(uint256 _tokenId, bool _isConvert, bool _isMint) external checkCompliance(_tokenId) {
        (uint256 cvgClaimable, ICommonStruct.TokenAmount[] memory tokenAmounts) = _claimCvgSdtRewards(_tokenId);

        sdtRewardReceiver.claimCvgSdtSimple(msg.sender, cvgClaimable, tokenAmounts, _isConvert, _isMint);

        emit ClaimCvgSdtMultiple(_tokenId, msg.sender);
    }

SdtRewardReceiver.claimCvgSdtSimple() calls _withdrawRewards() which swaps SDT rewards to cvg for the receiver using _poolCvgSDT.exchange(0, 1, rewardAmount, _poolCvgSDT.get_dy(0, 1, rewardAmount), receiver):

    function claimCvgSdtSimple(
        address receiver,
        uint256 totalCvgClaimable,
        ICommonStruct.TokenAmount[] memory totalSdtRewardsClaimable,
        bool isConvert,
        bool isMint
    ) external {
        require(cvgControlTower.isStakingContract(msg.sender), "NOT_STAKING");
        _withdrawRewards(receiver, totalCvgClaimable, totalSdtRewardsClaimable, isConvert, isMint);
    }

    ...

    function _withdrawRewards(
        address receiver,
        uint256 totalCvgClaimable,
        ICommonStruct.TokenAmount[] memory totalSdtRewardsClaimable,
        bool isConvert,
        bool isMint
    ) internal {
        /// @dev Mints accumulated CVG and claim StakeDao rewards
        IERC20 _sdt = sdt;
        if (totalCvgClaimable > 0) {
            cvg.mintStaking(receiver, totalCvgClaimable);
        }
        for (uint256 i; i < totalSdtRewardsClaimable.length; ) {
            uint256 rewardAmount = totalSdtRewardsClaimable[i].amount;
            if (rewardAmount > 0) {
                if (isConvert && totalSdtRewardsClaimable[i].token == _sdt) {
                    if (isMint) {
                        /// @dev Mint cvgSdt 1:1 via CvgToke contract
                        cvgSdt.mint(receiver, rewardAmount);
                    } else {
                        ICrvPoolPlain _poolCvgSDT = poolCvgSDT;
                        /// @dev Only swap if the returned amount in CvgSdt is gretear than the amount rewarded in SDT
                        _poolCvgSDT.exchange(0, 1, rewardAmount, _poolCvgSDT.get_dy(0, 1, rewardAmount), receiver);  // @audit accepts any exchange rate
                    }
                } else {
                    totalSdtRewardsClaimable[i].token.safeTransfer(receiver, rewardAmount);
                }
            }
            unchecked {
                ++i;
            }
        }
    }

As can be seen in sherlock-cvg/scripts/deployer/unit/XX_deployLiquidityCvgSdt.ts, the pool for cvg/cvgSDT is expected to be a curve plain pool (see also sherlock-cvg/resources/curve.ts). The code of the pool can be read on etherscan.

From the code of the pool we understand that _poolCvgSDT.get_dy(0, 1, rewardAmount) gets the expected output for a swap of token 0 (SDT) to token 1 (cvgSDT) of value rewardAmount. This value is used as the third parameter for _poolCvgSDT.exchange() which is _min_dy the minimal value expected to receive from the exchange.

This means that the exchange will compute the expected return value and give that as a minimum accepted return value. Despite what the @dev comment states, the swap will occur even when returned amount in cvgSDT is lower than the amount rewarded in SDT.

Impact

Anyone can sandwich a reward claim that should reward users with cvgSDT via swapping SDT for cvgSDT to steal the SDT reward via pool price manipulation.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Staking/StakeDAO/SdtStakingPositionService.sol#L396-L402

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Staking/StakeDAO/SdtRewardReceiver.sol#L98-L107

https://github.com/sherlock-audit/2023-11-convergence/blob/e894be3e36614a385cf409dc7e278d5b8f16d6f2/sherlock-cvg/contracts/Staking/StakeDAO/SdtRewardReceiver.sol#L213-L245

https://github.com/curvefi/curve-contract/blob/master/contracts/pool-templates/base/SwapTemplateBase.vy#L447

Tool used

Manual Review

Recommendation

Enforce a better return value from the swap on the pool and only use the pool if the returned value is more advantageous than minting. Something like:

        function _withdrawRewards(
        address receiver,
        uint256 totalCvgClaimable,
        ICommonStruct.TokenAmount[] memory totalSdtRewardsClaimable,
        bool isConvert,
        bool isMint
    ) internal {
        /// @dev Mints accumulated CVG and claim StakeDao rewards
        IERC20 _sdt = sdt;
        if (totalCvgClaimable > 0) {
            cvg.mintStaking(receiver, totalCvgClaimable);
        }
        for (uint256 i; i < totalSdtRewardsClaimable.length; ) {
            uint256 rewardAmount = totalSdtRewardsClaimable[i].amount;
            if (rewardAmount > 0) {
                if (isConvert && totalSdtRewardsClaimable[i].token == _sdt) {
                    if (isMint) {
                        /// @dev Mint cvgSdt 1:1 via CvgToke contract
                        cvgSdt.mint(receiver, rewardAmount);
                    } else {
                        ICrvPoolPlain _poolCvgSDT = poolCvgSDT;
                        /// @dev Only swap if the returned amount in CvgSdt is gretear than the amount rewarded in SDT
-                       _poolCvgSDT.exchange(0, 1, rewardAmount, _poolCvgSDT.get_dy(0, 1, rewardAmount), receiver);
+                       uint256 expectedCvgSDT = _poolCvgSDT.get_dy(0, 1, rewardAmount);
+                       if (expectedCvgSDT >= rewardAmount) {
+                           _poolCvgSDT.exchange(0, 1, rewardAmount, expectedCvgSDT, receiver);
+                       } else {
+                           cvgSdt.mint(receiver, rewardAmount);
+                       }
                    }
                } else {
                    totalSdtRewardsClaimable[i].token.safeTransfer(receiver, rewardAmount);
                }
            }
            unchecked {
                ++i;
            }
        }
    }

Duplicate of #180

sherlock-audit / 2023-11-convergence-judging

cducrest-brainbot - Withdrawing rewards will convert sdt to cvgSDT at any rate #204