Qoda DAO - Steadily Consulting Inc.

The swapBack function in the provided snippet may be vulnerable to manipulation because it relies on the balanceOf function to calculate the contract's balance. An attacker could potentially manipulate the contract's balance by sending tokens to the contract, causing the balanceOf function to return an inflated value. This manipulation might lead to unintended consequences in the distribution of tokens among liquidity providers and revenue streams.

Also, during its execution flow, the swapBack function calls swapTokensForCounterpart, that subsequently will call UniswapV2 contracts. These calls are going to revert, because of lack of slippage check when performing calls to UniswapV2.

- Function swapBack - src/QodaToken.sol [Lines: 378-418]

    function swapBack() private {
        uint256 contractBalance = balanceOf(address(this));
        uint256 totalTokensToSwap = tokensForLiquidity + tokensForRevStream1 + tokensForRevStream2;

        if (contractBalance == 0 || totalTokensToSwap == 0) {
            return;
        }

        if (contractBalance > swapTokensAtAmount * 20) {
            contractBalance = swapTokensAtAmount * 20;
        }

        // Halve the amount of liquidity tokens
        uint256 liquidityTokens = (contractBalance * tokensForLiquidity) / totalTokensToSwap / 2;
        uint256 amountToSwap = contractBalance - liquidityTokens;

        uint256 initialTokenBalance = IERC20(tokenAddress).balanceOf(address(this));

        swapTokensForCounterpart(amountToSwap);

        uint256 tokenBalance = IERC20(tokenAddress).balanceOf(address(this)) - initialTokenBalance;

        uint256 tokenForRevStream1 = tokenBalance * tokensForRevStream1 / (totalTokensToSwap - (tokensForLiquidity / 2));

        uint256 tokenForRevStream2 = tokenBalance * tokensForRevStream2 / (totalTokensToSwap - (tokensForLiquidity / 2));

        uint256 tokenForLiquidity = tokenBalance - tokenForRevStream1 - tokenForRevStream2;

        tokensForLiquidity = 0;
        tokensForRevStream1 = 0;
        tokensForRevStream2 = 0;

        IERC20(tokenAddress).safeTransfer(revStream2Wallet, tokenForRevStream2);

        if (liquidityTokens > 0 && tokenForLiquidity > 0) {
            addLiquidity(liquidityTokens, tokenForLiquidity);
            emit SwapAndLiquify(amountToSwap, tokenForLiquidity, tokensForLiquidity);
        }

        IERC20(tokenAddress).safeTransfer(revStream1Wallet, IERC20(tokenAddress).balanceOf(address(this)));
    }

- Function swapTokensForCounterpart - src/QodaToken.sol [Lines: 344-360]

    function swapTokensForCounterpart(uint256 tokenAmount) public {
        // generate the uniswap pair path of token -> counterpart token address
        address[] memory path = new address[](2);
        path[0] = address(this);
        path[1] = tokenAddress;

        _approve(address(this), address(uniswapV2Router), tokenAmount);

        // make the swap
        uniswapV2Router.swapExactTokensForTokensSupportingFeeOnTransferTokens(
            tokenAmount,
            0, // accept any amount of counterpart token
            path,
            address(this),
            block.timestamp
        );
    }

The current implementation of the swapBack function is prone to errors in calculations, because it relies on balanceOf method, instead of internal accounting. Also, all the calls to this function are going to revert, because of the subsequent call to swapTokensForCounterpart and to UniswapV2.

- Foundry test

    function test_QodaToken_swapBack_Incorrect_Implementation() public {
        vm.startPrank(attacker);
        /// attacker donates certain amount of QodaToken
        /// in order to cause errors on calculations
        qoda_token.transfer(address(qoda_token), 100 ether);
        vm.stopPrank();

        vm.startPrank(deployer);
        /// tries to call swapBack
        /// call will revert because lack of slippage
        /// protection in UniswapV2 call.
        vm.expectRevert();
        qoda_token.swapBack();
        vm.stopPrank();
    }

- Stack traces

  [115348] Halborn_QodaToken_test_Unit::test_QodaToken_swapBack_Insecure_reliance_on_balanceOf()
    ├─ [0] VM::startPrank(0x6E9972213BF459853FA33E28Ab7219e9157C8d02)
    │   └─ ← [Return] 
    ├─ [57229] QodaToken::transfer(QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], 100000000000000000000 [1e20])
    │   ├─ emit Transfer(from: 0x6E9972213BF459853FA33E28Ab7219e9157C8d02, to: QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], value: 100000000000000000000 [1e20])
    │   └─ ← [Return] true
    ├─ [36429] QodaToken::transfer(0x556330e8d92912cCf133851BA03abD2DB70Da404, 100000000000000000000 [1e20])
    │   ├─ emit Transfer(from: 0x6E9972213BF459853FA33E28Ab7219e9157C8d02, to: 0x556330e8d92912cCf133851BA03abD2DB70Da404, value: 100000000000000000000 [1e20])
    │   └─ ← [Return] true
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    ├─ [0] VM::startPrank(0x000000000000000000000000000000000000D00d)
    │   └─ ← [Return] 
    ├─ [6953] QodaToken::swapBack()
    │   └─ ← [Stop] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

To mitigate the current issues with the swapBack function, it is recommended to:

1. Use internal accounting for tracking deposits and values, instead of balanceOf.

2. Enforce a slippage parameter in the calls to UniswapV2⁣. Otherwise, the function calls will always revert.

Remediation Plan

SOLVED: The issue was addressed by refactoring or removing the affected functions in the QodaToken.sol contract. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

Introduced by the OpenZeppelin's contracts v5.x, the _update function is responsible for updating balances in the various state-changing operations of a common ERC-20 token, such as transfer, transferFrom, _mint and _burn. It is advised by the official documentation of the ERC20.sol contract (version 5) that custom logic in the transfer mechanisms should be implemented by overriding the _update function.

The QodaToken contract is inheriting from the aforementioned ERC20 v5.x by OpenZeppelin, and is implementing custom logic on transfers, such as fees and other conditions, by overriding the _update function. The head of the function is as follows:

- Function _update - src/QodaToken.sol [Lines: 257-261]

    function _update(address from, address to, uint256 amount) internal override {
        require(from != address(0), "ERC20: transfer from the zero address");
        require(to != address(0), "ERC20: transfer to the zero address");
        require(!blacklisted[from], "Sender blacklisted");
        require(!blacklisted[to], "Receiver blacklisted");

From the code snippet provided, it is possible to state that the overriding logic is effectively blocking all mint and burn operations, by requiring that both to and from parameters are different from the address(0).

While this could be due to a design decision, it is not standard practice to block all minting and burning functionalities, that in the presented scenario are only callable through super._update, by calling the parent _upgrade, non-modified function in the ERC20 contract.

To reproduce this vulnerability, it was simulated a burn transaction, that will fail because the overriding _update function is blocking all mint and burn operations.

- Forge test

    function test_QodaToken_Impossible_to_burn_tokens() public {
        vm.startPrank(seraph);
        /// expect to revert as custom implementation
        /// in the overriding `_update` function
        /// is blocking all transfers to address(0)
        vm.expectRevert();
        qoda_token.burn(seraph, 1 ether);
        vm.stopPrank();
    }

- Stack traces

  [11802] Halborn_QodaToken_test_Unit::test_QodaToken_Impossible_to_burn_tokens()
    ├─ [0] VM::startPrank(0x4CCeBa2d7D2B4fdcE4304d3e09a1fea9fbEb1528)
    │   └─ ← [Return] 
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← [Return] 
    ├─ [682] QodaToken::burn(0x4CCeBa2d7D2B4fdcE4304d3e09a1fea9fbEb1528, 1000000000000000000 [1e18])
    │   └─ ← [Revert] revert: ERC20: transfer to the zero address
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

Do not forbid transfers from or to the address(0) when overriding the _update function.

Remediation Plan

SOLVED: The issue was addressed by refactoring or removing the affected functions in the QodaToken.sol contract. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

In the RewardDistributor smart contract, functions like updateEpoch and updateAccountReward directly influence the financial integrity of the contract by affecting how rewards are computed and claimed. Leaving these functions without stringent access control or with restricted visibility could expose the contract to risks of state corruption or unintended financial discrepancies.

Both functions are called within the execution flow of the claimReward function in the RewardDistributor, and also in the VeQoda contract, which interacts with RewardDistributor through the _updateReward function, which calls updateAccountReward on each reward distributor registered in the _rewardDistributors set.

- Function claimReward - src/RewardDistributor.sol [Lines: 126-143]

    /// @notice Claim reward up till min(epochTarget, epochCurrent). Claiming on behalf of another person is allowed.
    /// @param account Account address which reward will be claimed
    /// @param epochTarget Ending epoch that account can claim up till, parameter exposed so that claiming can be done in step-wise manner to avoid gas limit breach
    function claimReward(address account, uint256 epochTarget) external {
        // Update unclaimed reward for an account before triggering reward transfer
        updateAccountReward(account, epochTarget);

        StakingStructs.AccountReward storage accountReward = accountRewards[account];
        if (accountReward.unclaimedReward > 0) {
            // Reset unclaimed reward before transfer to avoid re-entrancy attack
            uint256 reward = accountReward.unclaimedReward;
            accountReward.unclaimedReward = 0;
            IERC20(token).safeTransfer(account, reward);
            accountReward.claimedReward += reward;

            emit ClaimReward(account, epochTarget, reward);
        }
    }

- Function updateAccountReward - src/RewardDistributor.sol [Lines: 176-194]

    /// @notice Function to be called BEFORE veToken balance change for an account. Reward for an account will go into pendingReward
    /// @param account Account address for reward update to happen
    /// @param epochTarget Ending epoch that account reward can be updated up till, parameter exposed so that claiming can be done in step-wise manner to avoid gas limit breach
    function updateAccountReward(address account, uint256 epochTarget) public {
        // Update current epoch number if needed
        updateEpoch(epochTarget);

        // Make sure user cannot claim reward in the future
        if (epochTarget > epochCurrent) {
            epochTarget = epochCurrent;
        }

        // Calculate unclaimed reward
        uint256 unclaimedReward = getUnclaimedReward(account, epochTarget);

        StakingStructs.AccountReward storage reward = accountRewards[account];
        reward.unclaimedReward = unclaimedReward;
        reward.lastUpdateEpoch = epochTarget;
    }

- Function updateEpoch - src/RewardDistributor.sol [Lines: 145-174]

    /// @notice Check if it is first interaction after epoch starts, and fix amount of total ve token participated in previous epoch if so
    /// @param epochTarget Ending epoch that update will happen up till, parameter exposed so that update can be done in step-wise manner to avoid gas limit breach
    function updateEpoch(uint256 epochTarget) public {
        // Determine what epoch is current time in
        uint256 epochCurrentNew = getEpoch(block.timestamp);

        // If new epoch is on or before current epoch, either contract has not kick-started,
        // or this is not first interaction since epoch starts
        if (epochCurrentNew <= epochCurrent) {
            return;
        }

        // Epoch for current time now has surpassed target epoch, so limit epoch number
        if (epochCurrentNew > epochTarget) {
            epochCurrentNew = epochTarget;
        }

        // Take snapshot of total veToken since this is the first transaction in an epoch
        for (uint256 epoch = epochCurrent + 1; epoch <= epochCurrentNew;) {
            uint256 timeAtEpoch = getTimestamp(epoch);
            uint256 totalSupply = IVeQoda(veToken).totalVe(timeAtEpoch);
            totalVe.push(totalSupply);
            unchecked {
                epoch++;
            }
            // One emission for each epoch
            emit EpochUpdate(epoch, timeAtEpoch, totalSupply);
        }
        epochCurrent = epochCurrentNew;
    }

From the code snippets of the RewardDistributor contract, it is possible to observe that the functions updateEpoch and updateAccountRewards are not supposed to be called alone, but within the execution of the claimReward function.

Likewise, in the VeQoda Token contract, when users are interacting with the stake and unstake functions, they will call the internal _updateReward function in the VeQoda contract, that subsequently performs an external call to the updateAccountReward public function in the RewardDistributor contract.

- Function stake - src/VeQoda.sol [Lines: 80-127]

    /// @notice Stake token into contract
    /// @param account Account address for receiving staking reward
    /// @param method Staking method account used for staking
    /// @param amount Amount of token to stake
    function stake(address account, bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroStakeAmount();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(account);

        // if user exists, first update their cached veToken balance
        if (_users.contains(account)) {
            _updateVeTokenCache(account);
        }

        // Do token transfer from user to contract
        address token = _methodInfo[method].token;
        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

        // If user not exists before, create user info and add it to an array
        if (!_users.contains(account)) {
            _users.add(account);
        }

        // Update method info, all methods already has lastUpdateSec = block.timestamp in previous update, so only consider the newly added
        StakingStructs.StakingInfo storage info = _userInfo[account][method];
        StakingStructs.VeEmissionInfo[] storage veEmissions = _methodInfo[method].veEmissions;
        uint256 veEmissionLength = veEmissions.length;
        for (uint256 i = 0; i < veEmissionLength;) {
            // Time should be bounded by effective start and end time for current emission
            uint256 effectiveEnd = i < veEmissionLength - 1? veEmissions[i + 1].vePerDayEffective: type(uint256).max;
            uint256 time = _bounded(block.timestamp, veEmissions[i].vePerDayEffective, effectiveEnd);
            veEmissions[i].tokenAmount += amount;
            veEmissions[i].tokenAmountTime += amount * time;
            unchecked {
                i++;
            }
        }

        // Update user info
        info.amount += amount;
        info.lastUpdateSec = block.timestamp;

        // Emit the event
        emit Stake(account, method, token, amount);
    }

- Function unstake - src/VeQoda.sol [Lines: 129-206]

    /// @notice Unstake tokens, note that you will lose ALL your veToken if you unstake ANY amount with either method
    /// So to protect account interest, only sender can unstake, neither admin nor support can act on behalf in this process
    /// @param method Staking method user wish to unstake from
    /// @param amount Amount of tokens to unstake
    function unstake(bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroUnstakeAmount();
        }

        // User cannot over-unstake
        if (_userInfo[msg.sender][method].amount < amount) {
            revert CustomErrors.InsufficientBalance();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(msg.sender);

        // Reset user ve balance to 0 across all methods
        bool userStaked = false;
        uint256 methodsLength = _methods.length();
        for (uint256 i = 0; i < methodsLength;) {
            bytes32 methodBytes = _methods.at(i);

            StakingStructs.StakingInfo storage info = _userInfo[msg.sender][methodBytes];
            StakingStructs.MethodInfo storage methodInfo_ = _methodInfo[methodBytes];

            // Update method ve balance
            methodInfo_.totalVe -= info.amountVe;
            
            // For target staked method, reduce token amount across all ve emissions
            StakingStructs.VeEmissionInfo[] storage veEmissions = methodInfo_.veEmissions;
            uint256 veEmissionLength = veEmissions.length;
            for (uint256 j = 0; j < veEmissionLength;) {
                // Time should be bounded by effective start and end time for current emission
                uint256 effectiveEnd = j < veEmissionLength - 1? veEmissions[j + 1].vePerDayEffective: type(uint256).max;
                uint256 lastUpdateSec = _bounded(info.lastUpdateSec, veEmissions[j].vePerDayEffective, effectiveEnd);
                uint256 time = _bounded(block.timestamp, veEmissions[j].vePerDayEffective, effectiveEnd);

                if (methodBytes == method) {
                    // update token amount and timestamp-related cached value
                    veEmissions[j].tokenAmountTime -= info.amount * lastUpdateSec - (info.amount - amount) * block.timestamp;
                    veEmissions[j].tokenAmount -= amount;
                } else {
                    // update timestamp-related cached value
                    veEmissions[j].tokenAmountTime -= info.amount * (time - lastUpdateSec);
                }
                unchecked {
                    j++;
                }
            }

            // Update account balance and last update time
            info.amountVe = 0;
            info.lastUpdateSec = block.timestamp;
            if (methodBytes == method) {
                info.amount -= amount;
            }
            if (info.amount > 0) {
                userStaked = true;
            }

            unchecked {
                i++;
            }
        }

        // If user no longer stakes, remove user from array
        if (!userStaked) {
            _users.remove(msg.sender);
        }

        // Send back the withdrawn underlying
        address token = _methodInfo[method].token;
        IERC20(token).safeTransfer(msg.sender, amount);

        // Emit the event
        emit Unstake(msg.sender, method, token, amount);
    }

- Function _updateReward - src/VeQoda.sol [Lines: 387-398]

    /// @notice Function to be called to claim reward up to latest before making any ve balance change
    /// @param account Account address for receiving reward
    function _updateReward(address account) internal {
        uint256 rewardDistributorsLength = _rewardDistributors.length();
        for (uint256 i = 0; i < rewardDistributorsLength;) {
            address rewardDistributor = _rewardDistributors.at(i);
            IRewardDistributor(rewardDistributor).updateAccountReward(account, type(uint256).max);
            unchecked {
                i++;
            }
        }
    }

The identified vulnerabilities allow a malicious actor to invoke these unprotected functions arbitrarily, potentially leading to unauthorized manipulation of epoch management and reward calculations. This exploitation could result in several detrimental outcomes:

1. Epoch Manipulation: By calling updateEpoch without proper checks, an attacker could force the system into a new epoch prematurely, disrupting the intended timing for reward calculations and distributions. This premature transition could invalidate the correctness of ongoing epoch-based reward distributions, affecting all participating users.

   
   

2. Reward Calculation Errors: Unauthorized access to updateAccountReward could allow an attacker to repeatedly update reward accounts, potentially recalculating and claiming rewards based on manipulated or falsely reported staking data. This could lead to incorrect reward payouts, either denying rightful rewards to legitimate users or falsely inflating rewards to certain accounts.

   
   

3. Impact on VeQoda Contract: Since VeQoda relies on the integrity of the RewardDistributor for accurate veToken calculations and reward distributions, compromising the RewardDistributor directly affects the VeQoda contract's ability to maintain a reliable and trusted token supply and staking mechanism.

   
   

4. Financial and Reputational Damage: Beyond the immediate impact on token management and reward distributions, exploiting these vulnerabilities could lead to financial losses for users and damage the trust and reliability perceived in the platform, potentially causing long-term reputational harm to the project.

   
   

5. System-Wide Inconsistencies: Continuous exploitation of these vulnerabilities could lead to widespread inconsistencies across the staking and reward systems, making it challenging to restore integrity and confidence without significant rollback or system-wide updates, which may include pausing the contract, upgrading, or even a complete redeployment.

In order to reproduce this vulnerability, a simple call to updateEpoch and updateAccountRewards by a malicious actor would suffice to exploit the contract state and lead to undesired reward distribution and accounting.

- Foundry test

    function test_RewardDistributor_missing_access_control() public {
        vm.startPrank(seraph);
        /// calls `updateEpoch` with the current unix
        /// timestamp for the epoch.
        reward_distributor.updateEpoch(1715207594);
        /// calls `updateAccountReward` passing another
        /// user (ghost) and epoch `0` as parameters.
        reward_distributor.updateAccountReward(ghost, 0);
        vm.stopPrank();
    }

- Stack traces

  [273596] Halborn_QodaToken_test_Unit::test_RewardDistributor_missing_access_control()
    ├─ [0] VM::startPrank(0x4CCeBa2d7D2B4fdcE4304d3e09a1fea9fbEb1528)
    │   └─ ← [Return] 
    ├─ [251605] TransparentUpgradeableProxy::updateEpoch(1715207594 [1.715e9])
    │   ├─ [246660] RewardDistributor::updateEpoch(1715207594 [1.715e9]) [delegatecall]
    │   │   ├─ [46493] TransparentUpgradeableProxy::totalVe(1704067200 [1.704e9]) [staticcall]
    │   │   │   ├─ [41545] VeQoda::totalVe(1704067200 [1.704e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 2, epochStartTime: 1704067200 [1.704e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1704931200 [1.704e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1704931200 [1.704e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 3, epochStartTime: 1704931200 [1.704e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1705795200 [1.705e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1705795200 [1.705e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 4, epochStartTime: 1705795200 [1.705e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1706659200 [1.706e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1706659200 [1.706e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 5, epochStartTime: 1706659200 [1.706e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1707523200 [1.707e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1707523200 [1.707e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 6, epochStartTime: 1707523200 [1.707e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1708387200 [1.708e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1708387200 [1.708e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 7, epochStartTime: 1708387200 [1.708e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1709251200 [1.709e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1709251200 [1.709e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 8, epochStartTime: 1709251200 [1.709e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1710115200 [1.71e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1710115200 [1.71e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 9, epochStartTime: 1710115200 [1.71e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1710979200 [1.71e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1710979200 [1.71e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 10, epochStartTime: 1710979200 [1.71e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1711843200 [1.711e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1711843200 [1.711e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 11, epochStartTime: 1711843200 [1.711e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1712707200 [1.712e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1712707200 [1.712e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 12, epochStartTime: 1712707200 [1.712e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1713571200 [1.713e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1713571200 [1.713e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 13, epochStartTime: 1713571200 [1.713e9], totalSupply: 0)
    │   │   ├─ [7993] TransparentUpgradeableProxy::totalVe(1714435200 [1.714e9]) [staticcall]
    │   │   │   ├─ [7545] VeQoda::totalVe(1714435200 [1.714e9]) [delegatecall]
    │   │   │   │   └─ ← [Return] 0
    │   │   │   └─ ← [Return] 0
    │   │   ├─ emit EpochUpdate(epochNumber: 14, epochStartTime: 1714435200 [1.714e9], totalSupply: 0)
    │   │   └─ ← [Stop] 
    │   └─ ← [Return] 
    ├─ [8745] TransparentUpgradeableProxy::updateAccountReward(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0)
    │   ├─ [8297] RewardDistributor::updateAccountReward(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0) [delegatecall]
    │   │   └─ ← [Stop] 
    │   └─ ← [Return] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

To completely address the current issue with access control, the following modifications are recommended:

1. Change the visibility modifier of the current updateAccountReward and updateEpoch functions to internal or private, and also rename the functions to _updateAccountReward and _updateEpoch, to keep consistency with general convention for internal or private functions.

   
   

2. Create two new external functions (entry points), that will redirect incoming calls to the internal _updateAccountReward and _updateReward functions. Name these functions updateAccountReward and updateEpoch, and make sure that they can be externally called by the VeQoda contract. These external entry points must be access-controlled, so only authorized addresses (such as VeQoda token) can call it. Consider creating a modifier onlyVeQoda that will perform this verification.

Remediation Plan

RISK ACCEPTED: The risk associated with this finding was accepted.

Frontrunning is a common issue in decentralized finance (DeFi) applications, where malicious bots monitor the transaction mempool and execute transactions before others to capitalize on price fluctuations or arbitrage opportunities.

To mitigate the risk of frontrunning, and also to be able to make successful (non-reverting) calls to UniswapV2, it is crucial to implement slippage protection by setting an appropriate amountOutMin parameter when executing swaps or other transactions involving token exchanges. The amountOutMin parameter ensures that a transaction will only be executed if the user receives at least the specified minimum amount of output tokens.

- Function swapTokensForCounterpart - src/QodaToken.sol [Lines: 342-358]

    function swapTokensForCounterpart(uint256 tokenAmount) private {
        // generate the uniswap pair path of token -> counterpart token address
        address[] memory path = new address[](2);
        path[0] = address(this);
        path[1] = tokenAddress;

        _approve(address(this), address(uniswapV2Router), tokenAmount);

        // make the swap
        uniswapV2Router.swapExactTokensForTokensSupportingFeeOnTransferTokens(
            tokenAmount,
            0, // accept any amount of counterpart token
            path,
            address(this),
            block.timestamp
        );
    }

- Function addLiquidity - src/QodaToken.sol [Lines: 360-376]

    function addLiquidity(uint256 tokenAmount, uint256 counterpartAmount) private {
        // approve token transfer to cover all possible scenarios
        _approve(address(this), address(uniswapV2Router), tokenAmount);
        IERC20(tokenAddress).forceApprove(address(uniswapV2Router), counterpartAmount);

        // add the liquidity
        uniswapV2Router.addLiquidity(
            address(this),
            tokenAddress,
            tokenAmount,
            counterpartAmount,
            0, // slippage is unavoidable
            0, // slippage is unavoidable
            owner(),
            block.timestamp
        );
    }

The functions' strict enforcement of 0 slippage in UniswapV2 Router calls makes them vulnerable to sandwich attacks, front-running, and other MEV-based attacks exploiting arbitrage opportunities created by the lack of slippage protection.

When successfully exploited, the potential consequences include:

1. Financial loss: Attackers could manipulate the price of tokens during the execution of a trade, causing the victim to buy or sell at unfavorable rates, resulting in financial loss.

   
   

2. Arbitrage opportunities: Exploitation of the lack of slippage protection could allow attackers to frontrun transactions, taking advantage of arbitrage opportunities at the expense of the original transaction sender.

   
   

3. Transaction failure: In some cases, the lack of slippage tolerance could cause transactions to fail, as the expected price might not be achievable due to market fluctuations or manipulation by attackers.

   
   

4. Loss of trust: The exploitation of such vulnerabilities could lead to a loss of trust in the platform or contract, potentially damaging its reputation and user base.

To reproduce this vulnerability, simply perform calls to addLiquidity or swapTokensForCounterpart and the subsequent calls to UniswapV2 will revert, due to lack of slippage enforcement.

- Foundry test

    function test_calls_to_UniswapV2_without_slippage_protection() public {
        vm.startPrank(seraph);
        qoda_token.transfer(address(qoda_token), 1_000 ether);
        qoda_token.addLiquidity(100 ether, 1);
        qoda_token.swapTokensForCounterpart(100 ether);
        vm.stopPrank();
    }

- Stack traces

    ├─ [44112] QodaToken::swapTokensForCounterpart(100000000000000000000 [1e20])
    │   ├─ emit Approval(owner: QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], spender: 0x4752ba5DBc23f44D87826276BF6Fd6b1C372aD24, value: 100000000000000000000 [1e20])
    │   ├─ [20376] 0x4752ba5DBc23f44D87826276BF6Fd6b1C372aD24::swapExactTokensForTokensSupportingFeeOnTransferTokens(100000000000000000000 [1e20], 0, [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3, 0xaf88d065e77c8cC2239327C5EDb3A432268e5831], QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], 1714660034 [1.714e9])
    │   │   ├─ [7365] QodaToken::transferFrom(QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], 0xee84B3e9dDD405768B1027205BaD6D5A4791F5da, 100000000000000000000 [1e20])
    │   │   │   ├─ emit Transfer(from: QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], to: 0xee84B3e9dDD405768B1027205BaD6D5A4791F5da, value: 100000000000000000000 [1e20])
    │   │   │   └─ ← [Return] true
    │   │   ├─ [1250] 0xaf88d065e77c8cC2239327C5EDb3A432268e5831::balanceOf(QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3]) [staticcall]
    │   │   │   ├─ [553] 0x86E721b43d4ECFa71119Dd38c0f938A75Fdb57B3::balanceOf(QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3]) [delegatecall]
    │   │   │   │   └─ ← [Return] 1999999999999999999999 [1.999e21]
    │   │   │   └─ ← [Return] 1999999999999999999999 [1.999e21]
    │   │   ├─ [504] 0xee84B3e9dDD405768B1027205BaD6D5A4791F5da::getReserves() [staticcall]
    │   │   │   └─ ← [Return] 0x0000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000006633a2c2
    │   │   ├─ [674] QodaToken::balanceOf(0xee84B3e9dDD405768B1027205BaD6D5A4791F5da) [staticcall]
    │   │   │   └─ ← [Return] 200000000000000000000 [2e20]
    │   │   ├─ [3658] 0xee84B3e9dDD405768B1027205BaD6D5A4791F5da::swap(0, 0, QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], 0x)
    │   │   │   └─ ← [Revert] revert: UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT
    │   │   └─ ← [Revert] revert: UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT
    │   └─ ← [Revert] revert: UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT
    └─ ← [Revert] revert: UniswapV2: INSUFFICIENT_OUTPUT_AMOUNT

Enforce a minimum amount out, or create an uint parameter that could be used with off-chain calculations based on slippage tolerance.

Remediation Plan

SOLVED: The issue was addressed by refactoring or removing the affected functions in the QodaToken.sol contract. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

When developing a smart contract that interacts with Uniswap, avoid setting the deadline to block.timestamp or block.timestamp plus a fixed value.

The smart contract should independently verify that the user-submitted transaction is not outdated. This requires the contract to accept a deadline parameter from the user, pass it along to Uniswap, or revert the transaction if the deadline exceeds the current block.timestamp.

- Function swapTokensForCounterpart - src/QodaToken.sol [Lines: 342-358]

    function swapTokensForCounterpart(uint256 tokenAmount) private {
        // generate the uniswap pair path of token -> counterpart token address
        address[] memory path = new address[](2);
        path[0] = address(this);
        path[1] = tokenAddress;

        _approve(address(this), address(uniswapV2Router), tokenAmount);

        // make the swap
        uniswapV2Router.swapExactTokensForTokensSupportingFeeOnTransferTokens(
            tokenAmount,
            0, // accept any amount of counterpart token
            path,
            address(this),
            block.timestamp
        );
    }

A malicious block builder can withhold swap transactions and execute them later when it's advantageous for manipulating the price or offloading tokens onto the user at a disadvantageous price. Implementing a deadline parameter restricts the time frame during which an attacker can carry out such exploits.

Implementing a deadline parameter in order to restrict the time frame during which an attacker can carry out such exploits.

Remediation Plan

SOLVED: The issue was addressed by refactoring or removing the affected functions in the QodaToken.sol contract. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

RewardDistributor.sol

The state variable minReward in the RewardDistributor contract is never initialized, and therefore, there is no minimum value in QodaTokens enforced when calling the distribute function. This leads to a scenario where an attacker could potentially call the distribute function with a relatively small amount of QodaTokens to be distributed, effectively pushing a high amount of entries to the schedules array.

- Function distribute - src/RewardDistributor.sol [Lines: 112-115]

        // Avoid people distribute tiny amount of reward and create huge number of schedules
        if (amount < minReward) {
            revert CustomErrors.MinRewardNotMet();
        }

While this approach is a good practice to avoid spamming activities in the reward distribution schedule, an uninitialized variable minRewards would make this check ineffective, as minReward defaults to 0 when not initialized, which is the default value for the type.

This creates the adequate condition for grief attacks, specifically ones can lead the contract to a Denial of Service state, when iterating over an unbounded loop of schedules in the getUnclaimedReward function as follows:

- Function getUnclaimedReward - src/RewardDistributor.sol [Lines: 234-241]

        for (uint256 i = 0; i < schedules.length;) {
            StakingStructs.RewardSchedule memory schedule = schedules[i];
            (uint256 epochStart, uint256 epochEnd) = _getOverlap(
                schedule.epochStart,
                schedule.epochStart + schedule.epochNum - 1,
                reward.lastUpdateEpoch + 1,
                epochTarget
            );

This iteration over an unbounded loop can consume an excessive amount of gas, and will cause legitimate calls to getUnclaimedReward function to revert, leading the contract to a Denial of Service (DoS) state.

To reproduce this vulnerability, two steps must be taken:

1. An attacker must call distribute with a relatively small amount of QodaTokens, considering the current implementation of 18 decimals, to create a high volume of schedules.

2. Legitimate users will try to claim and calculate their rewards but won't be able, due to DoS state in the contract, caused by unbounded iteration over the schedules array.

- Foundry test

    function test_RewardDistributor_Denial_of_Service_min_Reward() public {
        vm.startPrank(attacker);
        /// the attacker approves sufficient amount
        /// so RewardDistributor can spend QodaTokens
        qoda_token.approve(address(reward_distributor), type(uint256).max);
        for (uint i = 1; i < 5000; i++) {
            /// attacker calls `distribute` many times,
            /// with a significant small amount of tokens
            /// on every call
            reward_distributor.distribute(i, block.timestamp, i);
        }
        vm.stopPrank();

        vm.startPrank(seraph);
        /// a legitimate user tries to call `getUnclaimedReward`
        /// in mainnet conditions, iterating over an array with
        /// 5000 will lead the contract to a DoS state, 
        /// and therefore, no user will be able to calculate
        /// claimable rewards, and therefore, these will not
        /// be credited to legitimate users.
        reward_distributor.getUnclaimedReward(seraph, block.timestamp);
        vm.stopPrank();
    }

- Stack traces omitted for brevity.

Enforce a value for _minReward upon contract's initialization.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

During the analysis of the VeQoda contract in-scope, it was identified that the contract is not initialized with valid entries for the _methodInfo mapping. This will directly lead to reverting calls to stake and unstake function, as they will try to call the safeTransferFrom function in the address(0), that would be the default value for uninitialized variables of type address.

- Function stake - src/VeQoda.sol [Lines: 84-99]

    function stake(address account, bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroStakeAmount();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(account);

        // if user exists, first update their cached veToken balance
        if (_users.contains(account)) {
            _updateVeTokenCache(account);
        }

        // Do token transfer from user to contract
        address token = _methodInfo[method].token;
        IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

- Function unstake - src/VeQoda.sol [Lines: 133-141]

    function unstake(bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroUnstakeAmount();
        }

        // User cannot over-unstake
        if (_userInfo[msg.sender][method].amount < amount) {
            revert CustomErrors.InsufficientBalance();
        }

In order to reproduce this vulnerability, simply call stake function on the VeQoda contract. As _methods is not initialized, it will try to perform a call to transferFrom in the address(0), which will revert.

- Foundry test

    function test_VeQoda_uninitialized_Methods() public {
        vm.startPrank(ghost);
        veqoda_token.stake(ghost, vanillaMethod, 137 ether);
        vm.stopPrank();
    }

- Stack traces

  [28438] Halborn_QodaToken_test_Unit::test_VeQoda_uninitialized_Methods()
    ├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← [Return] 
    ├─ [15919] TransparentUpgradeableProxy::stake(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0xdb4d4643dcf6d316600cbd896ad8099d4ffa127c9e8004f2e72652b1d9115134, 137000000000000000000 [1.37e20])
    │   ├─ [10958] VeQoda::stake(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0xdb4d4643dcf6d316600cbd896ad8099d4ffa127c9e8004f2e72652b1d9115134, 137000000000000000000 [1.37e20]) [delegatecall]
    │   │   ├─ [0] 0x0000000000000000000000000000000000000000::transferFrom(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, TransparentUpgradeableProxy: [0xbDb94598124b4A524Dc85Bb5dCe6878459df8c0E], 137000000000000000000 [1.37e20])
    │   │   │   └─ ← [Stop] 
    │   │   └─ ← [Revert] AddressEmptyCode(0x0000000000000000000000000000000000000000)
    │   └─ ← [Revert] AddressEmptyCode(0x0000000000000000000000000000000000000000)
    └─ ← [Revert] AddressEmptyCode(0x0000000000000000000000000000000000000000)

Initialize the _methods set during the initialization of the contract.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

The _updateReward internal function is called within the execution flow of important functions in the VeQoda contract, such as stake, unstake and _updateVeTokenCache. Due to lack of initialization of the _rewardDistributors address set, calls to _updateReward function will not function as intended, as it will try to iterate over an empty array.

- Function stake - src/VeQoda.sol [Lines: 84-90]

    function stake(address account, bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroStakeAmount();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(account);

- Function unstake - src/VeQoda.sol [Lines: 133-144]

    function unstake(bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroUnstakeAmount();
        }

        // User cannot over-unstake
        if (_userInfo[msg.sender][method].amount < amount) {
            revert CustomErrors.InsufficientBalance();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(msg.sender);

When the _updateReward internal function is called, it will iterate over the _rewardDistributors address set, performing external calls to Reward Distributors, specifically to the updateAccountReward function, which is a known method from the IRewardDistributor interface.

However, the VeQoda contract is initialized with an empty array (no data) for _rewardDistributors address set, meaning that it will be impossible to iterate over such set because i == 0. As consequence, the _updateReward internal function will not work as intended, specifically not performing external calls to reward distributors.

- Function _updateReward - src/VeQoda.sol [Lines: 389-398]

    function _updateReward(address account) internal {
        uint256 rewardDistributorsLength = _rewardDistributors.length();
        for (uint256 i = 0; i < rewardDistributorsLength;) {
            address rewardDistributor = _rewardDistributors.at(i);
            IRewardDistributor(rewardDistributor).updateAccountReward(account, type(uint256).max);
            unchecked {
                i++;
            }
        }
    }

The following steps can be performed to validate this vulnerability.

1. The function stake calls _updateRewards within its execution flow.

2. As the _rewardDistributors address set is never initialized, an external call to IRewardDistributor(rewardDistributor).updateAccountReward is never performed, effectively failing to update account reward information in the Reward Distributor contract.

   
   

- Foundry test

    function test_VeQoda_uninitialized_Distributors() public {
        vm.startPrank(ghost);
        qoda_token.approve(address(veqoda_token), 137 ether);
        veqoda_token.stake(ghost, vanillaMethod, 137 ether);
        vm.stopPrank();
    }

- Stack traces

  [260816] Halborn_QodaToken_test_Unit::test_VeQoda_uninitialized_Distributors()
    ├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← [Return] 
    ├─ [24808] QodaToken::approve(TransparentUpgradeableProxy: [0xbDb94598124b4A524Dc85Bb5dCe6878459df8c0E], 137000000000000000000 [1.37e20])
    │   ├─ emit Approval(owner: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, spender: TransparentUpgradeableProxy: [0xbDb94598124b4A524Dc85Bb5dCe6878459df8c0E], value: 137000000000000000000 [1.37e20])
    │   └─ ← [Return] true
    ├─ [237848] TransparentUpgradeableProxy::stake(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0xdb4d4643dcf6d316600cbd896ad8099d4ffa127c9e8004f2e72652b1d9115134, 137000000000000000000 [1.37e20])
    │   ├─ [232894] VeQoda::stake(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 0xdb4d4643dcf6d316600cbd896ad8099d4ffa127c9e8004f2e72652b1d9115134, 137000000000000000000 [1.37e20]) [delegatecall]
    │   │   ├─ [63013] QodaToken::transferFrom(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, TransparentUpgradeableProxy: [0xbDb94598124b4A524Dc85Bb5dCe6878459df8c0E], 137000000000000000000 [1.37e20])
    │   │   │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: TransparentUpgradeableProxy: [0xbDb94598124b4A524Dc85Bb5dCe6878459df8c0E], value: 137000000000000000000 [1.37e20])
    │   │   │   └─ ← [Return] true
    │   │   ├─ emit Stake(account: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, method: 0xdb4d4643dcf6d316600cbd896ad8099d4ffa127c9e8004f2e72652b1d9115134, token: QodaToken: [0xA6F12f7b68C6b86A3F951Ba5121145e5d3C6e2E3], amount: 137000000000000000000 [1.37e20])
    │   │   └─ ← [Stop] 
    │   └─ ← [Return] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

From the stack traces it is possible to observe that the external call to RewardDistributor contract is never performed, hence, evidencing the improper initialization of the _rewardDistributors address set.

The function visibility for the _addRewardDistributor function can be changed to public. This allows the function to be called within contract initialization.

Use the initialize function to call the _addRewardDistributor function within the contract initialization, building the proper state of at least one Reward Distributor.

The _addRewardDistributor notation for the external function can cause confusion, consider removing the underscore, so the final function signature is function addRewardDistributor(address rewardDistributor) public onlyRole(ROLE_ADMIN) {

Make sure to call addRewardDistributor in the initialization after attributing the roles. Otherwise, the function call will revert due to access control (onlyRole(ROLE_ADMIN)) .

Remediation Plan

RISK ACCEPTED: The risk associated with this finding was accepted.

There is a hard-cap on the maximum amount of QodaTokens each wallet can hold. Malicious users can donate tokens to other wallets in order to reach that limit earlier, without the wallet's owner triggering any buy operation.

- Function _update - src/QodaToken.sol [Lines: 278-288]

                //when buy
                if (automatedMarketMakerPairs[from] && !_isExcludedMaxTransactionAmount[to]) {
                    require(amount <= maxTransactionAmount, "Buy transfer amount exceeds the maxTransactionAmount.");
                    require(amount + balanceOf(to) <= maxWallet, "Max wallet exceeded");
                }
                //when sell
                else if (automatedMarketMakerPairs[to] && !_isExcludedMaxTransactionAmount[from]) {
                    require(amount <= maxTransactionAmount, "Sell transfer amount exceeds the maxTransactionAmount.");
                } else if (!_isExcludedMaxTransactionAmount[to]) {
                    require(amount + balanceOf(to) <= maxWallet, "Max wallet exceeded");
                }

The _update function is relying on the method balanceOf(address) to determine whether the account balance will reach the maximum allowed limit. Malicious users could brick buy operations by sending tokens to other users until the maxWallet is reached, effectively blocking legitimate buy transactions.

Implement internal accounting instead of relying on balanceOf for applying restrictions.

Remediation Plan

SOLVED: The issue was addressed by refactoring or removing the affected functions in the QodaToken.sol contract. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

The value of veEmissions[i].tokenAmountTime can be potentially manipulated by a malicious Proof-of-Stake (PoS) node operator. This variable is updated in a loop that iterates through the veEmissionLength. The value is calculated by multiplying the amount by the time, which is determined using the _bounded function and the current block's timestamp (block.timestamp).

- Function stake - src/VeQoda.sol [Lines: 110-119]

        for (uint256 i = 0; i < veEmissionLength;) {
            // Time should be bounded by effective start and end time for current emission
            uint256 effectiveEnd = i < veEmissionLength - 1? veEmissions[i + 1].vePerDayEffective: type(uint256).max;
            uint256 time = _bounded(block.timestamp, veEmissions[i].vePerDayEffective, effectiveEnd);
            veEmissions[i].tokenAmount += amount;
            veEmissions[i].tokenAmountTime += amount * time;
            unchecked {
                i++;
            }
        }

In a PoS consensus model, validators are responsible for creating and proposing new blocks. A malicious validator could manipulate the timing of blocks to their advantage, especially if they have a significant amount of staked tokens or can influence other validators. In this case, the malicious node operator could manipulate the block.timestamp value within the allowed bounds, causing the veEmissions[i].tokenAmountTime value to be artificially inflated or deflated.

Avoid relying on block.timestamp for emissions calculations.

Remediation Plan

RISK ACCEPTED: The risk associated with this finding was accepted.

The contracts VeQoda and RewardDistributor are upgradeable contracts, and therefore, it is recommended to use the __gap state variable to ensure that the state of the contract will be preserved across upgrades.

However, it was identified that the uint256[50] private __gap variable is misplaced on those contracts, as by convention, and to preserve the functionalities of the gap variable, it must be inserted at the end of the contract.

Place the uint256[50] private __gap; variable at the end of both VeQoda and RewardDistributor contracts.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

To enhance the security and flexibility of ownership transfer in your smart contracts, it is recommended to use a two-step ownership transfer process. This approach allows for a more controlled transfer of ownership, reducing the risk of unintended consequences or lost access to contract functionalities.

OpenZeppelin provides a convenient implementation of this pattern in the form of the Ownable2Step contract. By using Ownable2Step, you can separate the process of ownership transfer into two distinct steps: proposing a new owner and accepting the proposed ownership.

Implement Ownable2Step instead of Ownable, for enhanced security when transferring contract's ownership.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

During the assessment of the VeQoda.sol contract, it was identified that in certain edge cases, the unstake operation of the mentioned contract would revert because of a panic state induced by arithmetic underflow.

Especifically, the veEmissions[j].tokenAmountTime can underflow when amount is set to extremely low amounts, or if lastUpdateSec does not represent a valid multiplier for the value.

- Function unstake - src/VeQoda.sol [Lines: 182-183]

                    veEmissions[j].tokenAmountTime -=
                        info.amount * lastUpdateSec - (info.amount - amount) * block.timestamp;

It is recommended to adjust business logics around the affected function, so borderline values would not lead to a reverting transaction.

Remediation Plan

SOLVED: The client solved the issue through the commit hash f552c1e2a7d70b52d22593a5cd9fb7f36441ae77.

The Checks-Effects-Interactions pattern establishes a logical order of the execution of a function in order to avoid reentrancy attacks. Most important, to comply with the Checks-Effects-Interactions pattern, functions must perform external calls only when the state is already modified (effects), effectively acting against reentrancy vectors.

However, in the current contracts in-scope, it was identified three instances where the Checks-Effects-Interactions pattern is not respected.

- Function _update - src/QodaToken.sol [Lines: 298-307]

        if (
            canSwap && swapEnabled && !swapping && !automatedMarketMakerPairs[from] && !_isExcludedFromFees[from]
                && !_isExcludedFromFees[to]
        ) {
            swapping = true;

            swapBack();

            swapping = false;
        }

In the provided code, the swapBack function will ultimately perform external calls before all modifications to the contract's state are performed.

- Function claimReward - src/RewardDistributor.sol [Lines: 129-143]

    function claimReward(address account, uint256 epochTarget) external {
        // Update unclaimed reward for an account before triggering reward transfer
        updateAccountReward(account, epochTarget);

        StakingStructs.AccountReward storage accountReward = accountRewards[account];
        if (accountReward.unclaimedReward > 0) {
            // Reset unclaimed reward before transfer to avoid re-entrancy attack
            uint256 reward = accountReward.unclaimedReward;
            accountReward.unclaimedReward = 0;
            IERC20(token).safeTransfer(account, reward);
            accountReward.claimedReward += reward;

            emit ClaimReward(account, epochTarget, reward);
        }
    }

In the provided code, the claimReward function will ultimately perform external calls (through the updateAccountReward function) before all modifications to the contract's state are performed.

- Function stake - src/VeQoda.sol [Lines: 84-95]

    function stake(address account, bytes32 method, uint256 amount) external {
        if (amount <= 0) {
            revert CustomErrors.ZeroStakeAmount();
        }

        // Calculate unclaimed reward before balance update
        _updateReward(account);

        // if user exists, first update their cached veToken balance
        if (_users.contains(account)) {
            _updateVeTokenCache(account);
        }

In the provided snippet, the stake function will perform external calls, via _updateReward and _updateVeTokenCache functions, before all modifications to the contract's state are performed.

Make sure that external calls are only performed after state changes in the caller contract. Make sure to add reentrancy protections such as nonReentrant modifier, using the ReentrancyGuardUpgradeable or ReentrancyGuard contracts.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

Solc compiler version 0.8.20 switches the default target EVM version to Shanghai. The generated bytecode will include PUSH0 opcodes. The recently released Solc compiler version 0.8.25 switches the default target EVM version to Cancun, so it is also important to note that it also adds-up new opcodes such as TSTORE, TLOAD and MCOPY.

Be sure to select the appropriate EVM version in case you intend to deploy on a chain apart from mainnet like L2 chains that may not support PUSH0, TSTORE, TLOAD and/or MCOPY, otherwise deployment of your contracts will fail.

It is important to consider the targeted deployment chains before writing immutable contracts because, in the future, there might exist a need for deploying the contracts in a network that could not support new opcodes from Shanghai or Cancun EVM versions.

It is also recommended to not use floating ˆ pragmas.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

In the context of UniswapV2 liquidity pools (LPs), a lack of initial liquidity can make the pool vulnerable to inflation attacks, also known as first-deposit attacks. Although UniswapV2 partially mitigates this risk by adding initial liquidity by minting to address(0), low liquidity pools can still be subject to inflation and general price manipulation attacks.

In an inflation attack, an attacker provides a disproportionately large amount of one asset in the pool, causing the pool to mint an excessive number of LP tokens for the attacker. This results in a temporary inflation of the pool's perceived value, which the attacker can exploit by redeeming their inflated LP tokens for a larger share of the other asset in the pool.

These attacks can be particularly profitable when targeting Time-Weighted Average Price (TWAP) oracles. TWAP oracles, like the one employed in UniswapV2, calculate the average price of an asset over a specific time interval, which is meant to provide a more accurate and resistant price feed against short-term manipulations.

However, in low liquidity pools, an attacker can manipulate the TWAP by performing an inflation attack that lasts for the duration of the oracle's time window. This manipulation can result in a skewed average price, allowing the attacker to profit from the price discrepancy in other platforms or protocols that rely on the TWAP oracle for pricing information.

To mitigate the risk of inflation and price manipulation attacks, it is crucial to ensure that UniswapV2 pools maintain sufficient liquidity. This can be achieved by enforcing initial liquidity, right after LP pool deployment.

Remediation Plan

ACKNOWLEDGED: The issue was acknowledged.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and adds them to a special data structure known as “topics” instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all of the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

- src/IRewardDistributor.sol [Line: 18](src/IRewardDistributor.sol#L18)

	    event DistributeReward(address distributor, uint256 amount, uint256 startEpoch, uint256 numEpoch);

- src/IRewardDistributor.sol [Line: 21](src/IRewardDistributor.sol#L21)

	    event ClaimReward(address account, uint256 epochTarget, uint256 reward);

- src/IRewardDistributor.sol [Line: 24](src/IRewardDistributor.sol#L24)

	    event EpochUpdate(uint256 epochNumber, uint256 epochStartTime, uint256 totalSupply);

- src/IRewardDistributor.sol [Line: 27](src/IRewardDistributor.sol#L27)

	    event SetMinReward(address sender, uint256 newValue, uint256 oldValue);

- src/IRewardDistributor.sol [Line: 30](src/IRewardDistributor.sol#L30)

	    event SetMinEpoch(address sender, uint256 newValue, uint256 oldValue);

- src/IRewardDistributor.sol [Line: 33](src/IRewardDistributor.sol#L33)

	    event SetMaxEpoch(address sender, uint256 newValue, uint256 oldValue);

- src/IVeQoda.sol [Line: 21](src/IVeQoda.sol#L21)

	    event Stake(address indexed account, bytes32 method, address token, uint256 amount);

- src/IVeQoda.sol [Line: 24](src/IVeQoda.sol#L24)

	    event Unstake(address indexed account, bytes32 method, address token, uint256 amount);

- src/IVeQoda.sol [Line: 27](src/IVeQoda.sol#L27)

	    event SetStakingMethod(bytes32 stakingMethod, address token, uint256 vePerDay, uint256 vePerDayEffective);

- src/IVeQoda.sol [Line: 30](src/IVeQoda.sol#L30)

	    event AddRewardDistributor(address rewardDistributor);

- src/IVeQoda.sol [Line: 33](src/IVeQoda.sol#L33)

	    event RemoveRewardDistributor(address rewardDistributor);

- src/QodaToken.sol [Line: 96](src/QodaToken.sol#L96)

	    event ExcludeFromFees(address indexed account, bool isExcluded);

- src/QodaToken.sol [Line: 104](src/QodaToken.sol#L104)

	    event SwapAndLiquify(uint256 tokensSwapped, uint256 counterpartReceived, uint256 tokensIntoLiquidity);

Modify the declared events, attributing the indexed keyword for the important fields. This action will allow easier fetching of on-chain data through events.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

In Solidity, you can optimize the gas usage and improve the clarity of your smart contracts by setting appropriate visibility modifiers for your functions. Functions that are not intended to be called internally within the contract or its derived contracts can have their visibility modifier set to external.

The external visibility modifier restricts the function to be called only from outside the contract, preventing accidental or unintended internal calls. This restriction not only enhances the readability and maintainability of your code but also provides gas savings, as external calls are generally cheaper than public functions in terms of gas costs.

- src/QodaToken.sol [Line: 227]

	    function setAutomatedMarketMakerPair(address pair, bool value) public onlyOwner {

- src/QodaToken.sol [Line: 249]

	    function isExcludedFromFees(address account) public view returns (bool) {

- src/QodaToken.sol [Line: 253]

	    function isBlacklisted(address account) public view returns (bool) {

- src/QodaToken.sol [Line: 438]

	    function renounceBlacklist() public onlyOwner {

- src/QodaToken.sol [Line: 442]

	    function blacklist(address _addr) public onlyOwner {

- src/QodaToken.sol [Line: 452]

	    function blacklistLiquidityPool(address lpAddress) public onlyOwner {

- src/QodaToken.sol [Line: 462]

	    function unblacklist(address _addr) public onlyOwner {

- src/QodaToken.sol [Line: 466]

	    function setPreMigrationTransferable(address _addr, bool isAuthorized) public onlyOwner {

- src/RewardDistributor.sol [Line: 71]

	    function initialize(address token_, address veToken_, uint256 daysPerEpoch_, uint256 epochSystemStartSec_)

- src/VeQoda.sol [Line: 69]

	    function initialize(string memory name_, string memory symbol_) public initializer {

- src/VeQoda.sol [Line: 553]

	    function transfer(address to, uint256 amount) public virtual override(ERC20Upgradeable, IERC20) returns (bool) {

- src/VeQoda.sol [Line: 562]

	    function transferFrom(address from, address to, uint256 amount)

- src/VeQoda.sol [Line: 577]

	    function approve(address spender, uint256 amount)

- src/VeQoda.sol [Line: 591]

	    function totalSupply() public view virtual override(ERC20Upgradeable, IERC20) returns (uint256) {

- src/VeQoda.sol [Line: 597]

	    function balanceOf(address account) public view virtual override(ERC20Upgradeable, IERC20) returns (uint256) {

Change the function visibility modifier from public to external.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

In Solidity, it is essential to ensure the integrity and security of your smart contract by validating inputs before assigning them to state variables. One common oversight is the lack of input validation when assigning addresses to state variables, which can potentially lead to unintended consequences or security vulnerabilities.

By implementing proper input validation for addresses, you can prevent the assignment of invalid or malicious addresses to state variables, thereby reducing the risk of unexpected behavior in your smart contract.

- src/QodaToken.sol [Line: 110]

	        tokenAddress = tokenAddress_;

- src/QodaToken.sol [Line: 143]

	        revStream1Wallet = revStream1Wallet_; // set as revStream1 wallet

- src/QodaToken.sol [Line: 144]

	        revStream2Wallet = revStream2Wallet_; // set as revStream2 wallet

- src/QodaToken.sol [Line: 241]

	        revStream1Wallet = newRevStream1Wallet;

- src/QodaToken.sol [Line: 246]

	        revStream2Wallet = newWallet;

- src/RewardDistributor.sol [Line: 80]

	        token = token_;

- src/RewardDistributor.sol [Line: 81]

	        veToken = veToken_;

Implement proper input validation for addresses assigned to state variables by using require or if statements.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.

In programming, "magic numbers" refer to the use of unexplained numerical or string values directly in code, without any clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make your code more difficult to understand, maintain, and update.

To improve the readability and maintainability of your smart contracts, it is recommended to avoid using magic numbers and instead use named constants or variables to represent these values. By doing so, you provide clear context for the values, making it easier for developers to understand their purpose and significance.

- src/QodaToken.sol [Line: 130]

	        maxWallet = 10_000 * 1e18; // 1%

- src/QodaToken.sol [Line: 131]

	        swapTokensAtAmount = (totalSupply * 5) / 10000; // 0.05%

- src/QodaToken.sol [Line: 149]

	        excludeFromFees(address(0xdead), true);

- src/QodaToken.sol [Line: 153]

	        excludeFromMaxTransaction(address(0xdead), true);

- src/QodaToken.sol [Line: 181]

	        require(newAmount >= (totalSupply() * 1) / 100000, "Swap amount cannot be lower than 0.001% total supply.");

- src/QodaToken.sol [Line: 182]

	        require(newAmount <= (totalSupply() * 5) / 1000, "Swap amount cannot be higher than 0.5% total supply.");

- src/QodaToken.sol [Line: 188]

	        require(newNum >= ((totalSupply() * 5) / 1000) / 1e18, "Cannot set maxTransactionAmount lower than 0.5%");

- src/QodaToken.sol [Line: 189]

	        maxTransactionAmount = newNum  (10 * 18);

- src/QodaToken.sol [Line: 193]

	        require(newNum >= ((totalSupply() * 10) / 1000) / 1e18, "Cannot set maxWallet lower than 1.0%");

- src/QodaToken.sol [Line: 194]

	        maxWallet = newNum  (10 * 18);

- src/QodaToken.sol [Line: 211]

	        require(buyTotalFees <= 500, "Buy fees must be <= 5%.");

- src/QodaToken.sol [Line: 219]

	        require(sellTotalFees <= 500, "Sell fees must be <= 5%.");

- src/QodaToken.sol [Line: 273]

	            if (from != owner() && to != owner() && to != address(0) && to != address(0xdead) && !swapping) {

- src/QodaToken.sol [Line: 319]

	                fees = amount * sellTotalFees / 10000;

- src/QodaToken.sol [Line: 326]

	                fees = amount * buyTotalFees / 10000;

- src/QodaToken.sol [Line: 344]

	        address[] memory path = new address[](2);

- src/QodaToken.sol [Line: 445]

	            addr != address(uniswapV2Pair) && addr != address(0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D),

- src/RewardDistributor.sol [Line: 163]

	        for (uint256 epoch = epochCurrent + 1; epoch <= epochCurrentNew;) {

- src/RewardDistributor.sol [Line: 204]

	        return (timestamp - epochSystemStartSec) / (daysPerEpoch * 86400) + 1;

- src/RewardDistributor.sol [Line: 215]

	        return epochSystemStartSec + (epoch - 1)  daysPerEpoch  86400;

- src/RewardDistributor.sol [Line: 238]

	                schedule.epochStart + schedule.epochNum - 1,

- src/RewardDistributor.sol [Line: 239]

	                reward.lastUpdateEpoch + 1,

- src/VeQoda.sol [Line: 112]

	            uint256 effectiveEnd = i < veEmissionLength - 1? veEmissions[i + 1].vePerDayEffective: type(uint256).max;

- src/VeQoda.sol [Line: 163]

	                uint256 effectiveEnd = j < veEmissionLength - 1? veEmissions[j + 1].vePerDayEffective: type(uint256).max;

- src/VeQoda.sol [Line: 227]

	                    uint256 effectiveEnd = j < veEmissionLength - 1? veEmissions[j + 1].vePerDayEffective: type(uint256).max;

- src/VeQoda.sol [Line: 231]

	                    accountVe_ += info.amount  (timeEnd - lastUpdateSec)  vePerDay  1e18 / (86400  SCALE_FACTOR_VE_PER_DAY  (10 * method.tokenDecimal));

Avoid the use of magic numbers and replace them with named constants or variables, making it easier for developers to understand and work with the codebase.

Remediation Plan

SOLVED: The issue was addressed as recommended. The commit hash is 144cab46e7b09ac62604dd83996db4e6a2c5f083.