GimelSec - `SdtStakingPositionService.processSdtRewards` could record the wrong amount of sdt reward.

[sherlock-admin2]

GimelSec

medium

SdtStakingPositionService.processSdtRewards could record the wrong amount of sdt reward.

Summary

During the SdtStakingPositionService.processSdtRewards, it calls buffer.pullRewards to pull the reward and the reward amount will be recorded in _sdtRewardsByCycle[_cvgStakingCycle][erc20Id]. However, if the array of the reward assets contains duplicates, the recorded amount could go wrong.

Vulnerability Detail

In processSdtRewards, it writes _sdtRewardsByCycle[_cvgStakingCycle][erc20Id] in a for loop. If there are duplicate erc20 tokens. The newer amount will overwrite the older amount.
https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Staking/StakeDAO/SdtStakingPositionService.sol#L570

    function processSdtRewards() external {
        …

        /// @dev call and returns tokens and amounts returned in rewards by the gauge
        ICommonStruct.TokenAmount[] memory _rewardAssets = buffer.pullRewards(msg.sender);

        for (uint256 i; i < _rewardAssets.length; ) {
            IERC20 _token = _rewardAssets[i].token;
            uint256 erc20Id = _tokenToId[_token];
            if (erc20Id == 0) {
                uint256 _numberOfSdtRewards = ++numberOfSdtRewards;
                _tokenToId[_token] = _numberOfSdtRewards;
                erc20Id = _numberOfSdtRewards;
            }

            _sdtRewardsByCycle[_cvgStakingCycle][erc20Id] = ICommonStruct.TokenAmount({
                token: _token,
                amount: _rewardAssets[i].amount
            });
            unchecked {
                ++i;
            }
        }

        _cycleInfo[_cvgStakingCycle].isSdtProcessed = true;

        emit ProcessSdtRewards(_cvgStakingCycle, msg.sender, _rewardAssets);
    }

In SdtBuffer.pullRewards, it doesn’t check whether reward tokens and bribe tokens have duplicates. https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Rewards/StakeDAO/SdtBuffer.sol#L74

    function pullRewards(address processor) external returns (ICommonStruct.TokenAmount[] memory) {
        …

        /// @dev Instantiate the returned array with the maximum potential length
        ICommonStruct.TokenAmount[] memory tokenAmounts = new ICommonStruct.TokenAmount[](
            rewardAmount + bribeTokens.length
        );

        /// @dev counter used for the actual index of the array, we need it as we remove 0 amounts from our returned array
        uint256 counter;
        address _processor = processor;
        for (uint256 j; j < rewardAmount; ) {
            /// @dev Retrieve the reward asset on the gauge contract
            IERC20 token = _gaugeAsset.reward_tokens(j);
            /// @dev Fetches the balance in this reward asset
            uint256 balance = token.balanceOf(address(this));
            /// @dev distributes if the balance is different from 0
            if (balance != 0) {
                uint256 fullBalance = balance;
                /// @dev Some fees are taken in SDT
                if (token == _sdt) {
                    ISdtFeeCollector _feeCollector = cvgControlTower.sdtFeeCollector();
                    /// @dev Fetches the % of fees to take & compute the amount compare to the actual balance
                    uint256 sdtFees = (_feeCollector.rootFees() * balance) / 100_000;
                    balance -= sdtFees;
                    /// @dev Transfers SDT fees to the FeeCollector
                    token.transfer(address(_feeCollector), sdtFees);
                }

                /// @dev send rewards to claimer
                uint256 claimerRewards = (fullBalance * _processorRewardsPercentage) / DENOMINATOR;
                if (claimerRewards > 0) {
                    token.transfer(_processor, claimerRewards);
                    balance -= claimerRewards;
                }

                /// @dev transfers the balance (or the balance - fees for SDT) minus claimer rewards (if any) to the staking contract
                token.transfer(sdtRewardsReceiver, balance);
                /// @dev Pushes in the TokenAmount array
                tokenAmounts[counter++] = ICommonStruct.TokenAmount({token: token, amount: balance});
            }
            /// @dev else reduces the length of the array to not return some useless 0 TokenAmount structs
            else {
                // solhint-disable-next-line no-inline-assembly
                assembly {
                    mstore(tokenAmounts, sub(mload(tokenAmounts), 1))
                }
            }

            unchecked {
                ++j;
            }
        }

        /// @dev Iterates through bribes assets, transfers them to the staking and push in the TokenReward amount
        for (uint256 j; j < bribeTokens.length; ) {
            IERC20 token = bribeTokens[j].token;
            uint256 amount = bribeTokens[j].amount;
            /// @dev Fetches the bribe token balance
            if (amount != 0) {
                /// @dev Pushes in the TokenAmount array
                tokenAmounts[counter++] = ICommonStruct.TokenAmount({token: token, amount: amount});
            }
            /// @dev else reduces the length of the array to not return some useless 0 TokenAmount structs
            else {
                // solhint-disable-next-line no-inline-assembly
                assembly {
                    mstore(tokenAmounts, sub(mload(tokenAmounts), 1))
                }
            }
            unchecked {
                ++j;
            }
        }

        return tokenAmounts;
    }

Impact

If _sdtRewardsByCycle is wrong, the users cannot receive the full amount of rewards.

Code Snippet

https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Staking/StakeDAO/SdtStakingPositionService.sol#L570 https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Rewards/StakeDAO/SdtBuffer.sol#L74

Tool used

Manual Review

Recommendation

SdtStakingPositionService.processSdtRewards should take care of duplicates in _rewardAssets

Duplicate of #182