Security Testing Report for Radiant V2

Report Manifest

Version History

1. Introduction

1.1 About Security Testing

We are invited by Radiant Capital to conduct a security testing (as the red team) for the Radiant V2's smart contracts to identify potential risks. As a responsible team, Radiant Capital takes security seriously. Hence the team decided to put more efforts into securing those smart contracts, while they have been audited by multiple security companies ^1.

Note that security testing is different from security audit in both goals and requirements. Specifically, security testing aims to discover extra/unusual vulnerable points by mimicking attackers to break the program/protocol, while security audit aims to give a relatively comprehensive security check by enumerating the possible attack surfaces. As such, security testing might not be able to cover some complicated logic bugs that could be identified by security audit due to the limited time and resource.

1.2 About Target Contracts

The target repository is Radiant_v2.1.1. The commit SHA values during the security testing are shown in the following. Our report is responsible for the only initial version (i.e., Version 1), as well as new codes (in the following versions) to fix issues in the report.

Note that, this report only covers smart contracts under the radiant_v2.1.1/contracts folder of this repository, including:

• bounties
• deployments
• flashloan
• leverage
• lock
• oracles
• staking
• zap
• eligibility
• misc
• oft
• protocol
• stargate

After the update in Version 8, the files covered in this security testing include:

• lending/AaveOracle.sol
• lending/AaveProtocolDataProvider.sol
• lending/ATokensAndRatesHelper.sol
• lending/StableAndVariableTokensHelper.sol
• lending/UiPoolDataProviderV2V3.sol
• lending/UiPoolDataProvider.sol
• lending/WETHGateway.sol
• lending/WalletBalanceProvider.sol
• lending/configuration
• lending/flashloan
• lending/lendingpool
• lending/tokenization
• radiant/accessories
• radiant/eligibility
• radiant/oracles
• radiant/staking
• radiant/token
• radiant/zap

1.3 Security Model

To evaluate the risk, we follow the standards or suggestions that are widely adopted by both industry and academy, including OWASP Risk Rating Methodology ^2 and Common Weakness Enumeration ^3. The overall severity of the risk is determined by likelihood and impact. Specifically, likelihood is used to estimate how likely a particular vulnerability can be uncovered and exploited by an attacker, while impact is used to measure the consequences of a successful exploit.

In this report, both likelihood and impact are categorized into two ratings, i.e., high and low respectively, and their combinations are shown in Table 1.1.

Accordingly, the severity measured in this report are classified into three categories: High, Medium, Low. For the sake of completeness, Undetermined is also used to cover circumstances when the risk cannot be well determined.

Furthermore, the status of a discovered item will fall into one of the following four categories:

• Undetermined No response yet.

• Acknowledged The item has been received by the client, but not confirmed yet.

• Confirmed The item has been recognized by the client, but not fixed yet.

• Fixed The item has been confirmed and fixed by the client.

2. Automated Security Testing

2.1 Automated Static Security Testing

We use our in-house static analysis tool based on Slither to check the existence of the vulnerabilities. After checking the results manually, no issues were found. Detailed testing results can be found in Table 4.1 in Appendix.

2.2 Automated Dynamic Security Testing

We utilize fuzzing techniques to test the robustness, reliability, and precision of the target contracts. Specifically, the initial seed in the fuzzing process is determined based on the function semantics and contract test scripts. To simulate the on chain environment, we also maintain a set of addresses that have interacted with the contract LendingPool and MultiFeeDistribution.

Our fuzzer also considers function semantics during transaction sequence generation. For example, function stake in contract MultiFeeDistribution and function deposit in contract LendingPool are likely to be invoked first in the sequence. The mutation to the function parameters and sequence is guided by the contract code coverage. If a certain parameter or sequence reaches higher code coverage, it will has higher priority to be mutated in the next fuzzing round. To explore some path constrained by magic number, we collect the values read from storage (i.e., the SLOAD instruction) at runtime and use them to generate function parameters during the mutation process.

In total, we generate 100,000 test cases and utilize 31 oracles, which is used to detect if a failure has occurred. For each test case, it contains 30 transactions with specified orders. Finally, we discovered one critical issue (i.e., Section 3.2.6), which is also discovered in our manual security testing process. Detailed testing results can be found in Tables 4.2, 4.3, and 4.4 in Appendix.

3. Manual Security Testing

We involve manual efforts to understand the overall design and interactions between different modules, and then conduct the security testing based on our know-how of potential attack surfaces derived from our previous research and experience.

In total, we find seventeen potential issues. Besides, we have three recommendations and one notes as follows:

• High Risk: 2

• Medium Risk: 8

• Low Risk: 7

• Recommendations: 3

• Notes: 1

The details are provided in the following sections.

3.1 Software Security

3.1.1 Potential Issue 1: No Reserved Interface for Resetting Function Pointers

Description Three functions, getLpMfdBounty(), getChefBounty(), and getAutoCompoundBounty(), are invoked through function pointers in contract BountyManager. Meanwhile, the inheritance from OwnableUpgradable shows that this contract would be the implementation of a proxy. This indicates that the implementation contract can be upgraded in the future, which brings an issue related to the function pointers.

function initialize(
        address _rdnt,
        address _weth,
        address _lpMfd,
        address _mfd,
        address _chef,
        address _priceProvider,
        address _eligibilityDataProvider,
        uint256 _hunterShare,
        uint256 _baseBountyUsdTarget,
        uint256 _maxBaseBounty,
        uint256 _bountyBooster
    ) external initializer {
        require(_rdnt != address(0));
        require(_weth != address(0));
        require(_lpMfd != address(0));
        require(_mfd != address(0));
        require(_chef != address(0));
        require(_priceProvider != address(0));
        require(_eligibilityDataProvider != address(0));
        require(_hunterShare <= 10000);
        require(_baseBountyUsdTarget != 0);
        require(_maxBaseBounty != 0);
 
        rdnt = _rdnt;
        weth = _weth;
        lpMfd = _lpMfd;
        mfd = _mfd;
        chef = _chef;
        priceProvider = _priceProvider;
        eligibilityDataProvider = _eligibilityDataProvider;
 
        HUNTER_SHARE = _hunterShare;
        baseBountyUsdTarget = _baseBountyUsdTarget;
        bountyBooster = _bountyBooster;
        maxBaseBounty = _maxBaseBounty;
 
        bounties[1] = getLpMfdBounty;
        bounties[2] = getChefBounty;
        bounties[3] = getAutoCompoundBounty;
        bountyCount = 3;
 
        slippageLimit = 10;
        minDLPBalance = uint256(5).mul(10 ** 18);
 
 
        __Ownable_init();
        __Pausable_init();
    } 

Listing 3.1: BountyManager.sol

Impact When the offsets of the above mentioned three functions are changed, the function pointers cannot work as expected and the whole logic of the contract can be changed.

Suggestion The contract should provide interfaces for resetting the function pointers.

3.2 DeFi Security

3.2.1 Potential Issue 2: Improper Calculation of the Oracle

Description The function consult() in contract ComboOracle is is used to compute the average price from several sources. In the implementation of version 1, it uses arithmetic mean to calculate the final price, which can be manipulated by influencing one of the source oracles.

function consult() public view override returns (uint256 price) {
        require(sources.length != 0);

        uint256 sum;
        for (uint256 i = 0; i < sources.length; i++) {
            uint256 price = sources[i].consult();
            require(price != 0, "source consult failure");
            sum = sum.add(price);
        }
        price = sum.div(sources.length);
    }

Listing 3.2: ComboOracle.sol

In the implementation of version 4, when the average price is greater than the lowest price×1.025, lowest price will be returned. However, the return value can still be manipulated if the result returned from one of the source oracles is abnormally low.

/**
    * @notice Calculated price
    * @return price Average price of several sources.
    */
   function consult() public view override returns (uint256 price) {
       require(sources.length != 0);

       uint256 sum;
       uint256 lowestPrice;
       for (uint256 i = 0; i < sources.length; i++) {
           uint256 price = sources[i].consult();
           require(price != 0, "source consult failure");
           if (lowestPrice == 0) {
               lowestPrice = price;
           } else {
               lowestPrice = lowestPrice > price ? price : lowestPrice;
           }
           sum = sum.add(price);
       }
       price = sum.div(sources.length);
       price = price > ((lowestPrice * 1025) / 1000) ? lowestPrice : price;
   }

Listing 3.3: ComboOracle.sol

Impact The price returned from ComboOracle can be manipulated, which allows the attacker to gain profit from it.

Suggestion We suggest using medium value instead of the average value. If there are only two source oracles and a rather big difference occurs, it is more reasonable to revert the transaction when the avarage price is rather larger than the lowest price.

Feedback There will be only two source oracles. If there is a rather big difference occurs, we'll use an OZ Defender Sentinel to pause associated contracts.

Note The contract ComboOracle is removed and no longer used.

3.2.2 Potential Issue 3: Potential Drain of Funds through BaseBounty

Description A user can lock tokens (i.e., RDNT) for a fixed duration to earn rewards. When the lock expires, other users can invoke the function executeBounty() to relock the tokens for this user to earn the BaseBounty if this user has the AutoRelock enabled. During the relocking process, the expired locks will be cleared and restaked into the pool in the internal function _cleanWithdrawableLocks(). However, there is a variable maxLockWithdrawPerTxn limiting the maximum number of locks that can be cleared. In this case, uncleared expired locks may still exist even after the function executeBounty() being executed. This can further bypass the check in line 106 of function claimBounty() in the contract MFDPlus. The issueBaseBounty will be set as true and returned back.

**
    * @notice Withdraw all lockings tokens where the unlock time has passed
    */
   function _cleanWithdrawableLocks(
       address user,
       uint256 totalLock,
       uint256 totalLockWithMultiplier
   ) internal returns (uint256 lockAmount, uint256 lockAmountWithMultiplier) {
       LockedBalance[] storage locks = userLocks[user];

       if (locks.length != 0) {
           uint256 length = locks.length <= maxLockWithdrawPerTxn ? locks.length : maxLockWithdrawPerTxn;
           for (uint256 i = 0; i < length; ) {
               if (locks[i].unlockTime <= block.timestamp) {
                   lockAmount = lockAmount.add(locks[i].amount);
                   lockAmountWithMultiplier = lockAmountWithMultiplier.add(
                       locks[i].amount.mul(locks[i].multiplier)
                   );
                   locks[i] = locks[locks.length - 1];
                   locks.pop();
                   length = length - 1;
               } else {
                   i = i + 1;
               }
           }
           if (locks.length == 0) {
               lockAmount = totalLock;
               lockAmountWithMultiplier = totalLockWithMultiplier;
               delete userLocks[user];

               userlist.removeFromList(user);
           }
       }
   }

Listing 3.4: MultiFeeDistribution.sol

Specifically, the attacker can stake 1 wei token with the same expiration time for multiple times, which is rather larger than maxLockWithdrawPerTxn. After that, the attacker can set the action as getLpMfdBounty and invoke executeBounty() repeatedly. As the amount of cleared locks is limited by the maxLockWithdrawPerTxn, the BaseBounty in the contract BountyManager can be drained by the attacker.

Impact The attacker can drain all funds in the contract BountyManager in one transaction, leading to the disruption of designed bounty mechanisms.

Suggestion Ensure the function _cleanWithdrawableLocks() can clear all expired locks and set a minimum staking amount in function _stake().

3.2.3 Potential Issue 4: Potential Invalid Emission Schedules

Description In the contract ChefIncentivesController , function setEmissionSchedule() is invoked by the owner to set schedules for different rewards rates. In this case, the start time for each schedule (_startTimeOffsets[i] + startTime) should be validated to be larger than the current timestamp. However, it only checks the first element in _startTimeOffsets, which is not enough. Furthermore, the _startTimeOffsets[i] is converted from uint256 to uint128 when it’s being added to emissionSchedule, which can be truncated if the original input is too large.

function setEmissionSchedule(
        uint256[] calldata _startTimeOffsets,
        uint256[] calldata _rewardsPerSecond
    ) external onlyOwner {
        uint256 length = _startTimeOffsets.length;
        require(length > 0 && length == _rewardsPerSecond.length, "empty or mismatch params");
        if (startTime > 0) {
            require(_startTimeOffsets[0] > block.timestamp.sub(startTime), "invalid start time");
        }
 
        for (uint256 i = 0; i < length; i++) {
            emissionSchedule.push(
                EmissionPoint({
                    startTimeOffset: uint128(_startTimeOffsets[i]),
                    rewardsPerSecond: uint128(_rewardsPerSecond[i])
                })
            );
        }
        emit EmissionScheduleAppended(_startTimeOffsets, _rewardsPerSecond);
    } 

Listing 3.5: ChefIncentivesController.sol

Impact If _startTimeOffsets is not in ascending order, some promised rewards will not be distributed to the users. If _startTimeOffsets[i] is beyond the range of uint128 , an invalid emission schedule will be added.

Suggestion Ensure the _startTimeOffsets is in ascending order and all elements are within the uint128 range.

3.2.4 Potential Issue 5: Skippable Emission schedules

Description In contract ChefIncentivesController, the function setScheduleRewardsPerSecond() will iterate emissionSchedule to locate the target schedule with the largest index that has already started, and update the reward rate accordingly. However, in this case, some emission schedules may be skipped.

function setScheduledRewardsPerSecond() internal {
		if (!persistRewardsPerSecond) {
			uint256 length = emissionSchedule.length;
			uint256 i = emissionScheduleIndex;
			uint128 offset = uint128(block.timestamp.sub(startTime));
			for (; i < length && offset >= emissionSchedule[i].startTimeOffset; i++) {}
			if (i > emissionScheduleIndex) {
				emissionScheduleIndex = i;
				_massUpdatePools();
				rewardsPerSecond = uint256(emissionSchedule[i - 1].rewardsPerSecond);
			}
		}
	}

Listing 3.6: ChefIncentivesController.sol

Impact If the function setScheduledRewardsPerSecond() is not invoked for a long time, some promised rewards may not be distributed to the users.

Suggestion The function setScheduledRewardsPerSecond() is invoked inside function claim() and _handleActionAfterForToken(), so the only way the emissions schedule would be skipped would be for no people to interact with the protocol during an emissions epoch.

3.2.5 Potential Issue 6: Changeable Exchange Rate during Migration

Description The contract Migration is implemented for users to exchange from tokenV1 to tokenV2 with a specified exchangeRate. However, during the process of migration, this exchangeRate can still be adjusted by the owner via the function setExchangeRate().

/**
    * @notice Migrate from V1 to V2
    * @param amount of V1 token
    */
   function exchange(uint256 amount) external whenNotPaused {
       uint256 v1Decimals = tokenV1.decimals();
       uint256 v2Decimals = tokenV2.decimals();

       uint256 outAmount = amount.mul(1e4).div(exchangeRate).mul(10**v2Decimals).div(10**v1Decimals);
       tokenV1.safeTransferFrom(_msgSender(), address(this), amount);
       tokenV2.safeTransfer(_msgSender(), outAmount);

       emit Migrate(_msgSender(), amount, outAmount);
   }

Listing 3.7: Migration.sol

Impact It will be unfair to the other users, if the exchangeRate is changed during the migration process.

Suggestion Once the migration starts, the exchangeRate should be fixed.

3.2.6 Potential Issue 7: Improper Implementation of \_transfer() (I)

Description In contract IncentivizedERC20, the function _transfer() does not consider the situation that the sender and the recipient can be the same account (so-called self transfer). Specifically, if the sender equals to the recipient, the sender’s balance will be overwritten when updating the recipient’s balance. In this case, the hacker is able to increase his/her own balance infinitely by transferring to his/her own account repeatedly.

function _transfer(
        address sender,
        address recipient,
        uint256 amount
      ) internal virtual {
        require(sender != address(0), 'ERC20: transfer from the zero address');
        require(recipient != address(0), 'ERC20: transfer to the zero address');
    
        _beforeTokenTransfer(sender, recipient, amount);
    
        uint256 senderBalance = _balances[sender].sub(amount, 'ERC20: transfer amount exceeds balance');
        uint256 recipientBalance = _balances[recipient].add(amount);
    
        if (address(_getIncentivesController()) != address(0)) {
          // uint256 currentTotalSupply = _totalSupply;
          _getIncentivesController().handleActionBefore(sender);
          if (sender != recipient) {
            _getIncentivesController().handleActionBefore(recipient);
          }
        }
    
        _balances[sender] = senderBalance;
        _balances[recipient] = recipientBalance;
    
        if (address(_getIncentivesController()) != address(0)) {
          uint256 currentTotalSupply = _totalSupply;
          _getIncentivesController().handleActionAfter(sender, senderBalance, currentTotalSupply);
          if (sender != recipient) {
            _getIncentivesController().handleActionAfter(recipient, recipientBalance, currentTotalSupply);
          }
        }
      }

Listing 3.8: IncentivizedERC20.sol

Impact Tokens can be minted infinitely.

Suggestion Implement the function _transfer() properly. For example, the standard _transfer() implementation of ERC20 in OpenZeppelin.

_balances[sender] = _balances[sender].sub(amount, 'ERC20: transfer amount exceeds balance');
_balances[recipient] = _balances[recipient].add(amount);

Listing 3.9: ERC20.sol in OpenZeppelin

3.2.7 Potential Issue 8: Lack of Check on Period in UniV2TwapOracle

Description In the contract UniV2TwapOracle, the attribute _period is not validated in the function initialize() and setPeriod().

function initialize(
        address _pair,
        address _rdnt,
        address _ethChainlinkFeed,
        uint _period,
        uint _consultLeniency,
        bool _allowStaleConsults
    ) external initializer {
        __Ownable_init();

        pair = IUniswapV2Pair(_pair);
        token0 = pair.token0();
        token1 = pair.token1();
        price0CumulativeLast = pair.price0CumulativeLast(); // Fetch the current accumulated price value (1 / 0)
        price1CumulativeLast = pair.price1CumulativeLast(); // Fetch the current accumulated price value (0 / 1)
        uint112 reserve0;
        uint112 reserve1;
        (reserve0, reserve1, blockTimestampLast) = pair.getReserves();
        require(reserve0 != 0 && reserve1 != 0, 'UniswapPairOracle: NO_RESERVES'); // Ensure that there's liquidity in the pair

        PERIOD = _period;
        CONSULT_LENIENCY = _consultLeniency;
        ALLOW_STALE_CONSULTS = _allowStaleConsults;

        baseInitialize(_rdnt, _ethChainlinkFeed);
    }

    function setPeriod(uint _period) external onlyOwner {
        PERIOD = _period;
    }

Listing 3.10: UniV2TwapOracle.sol

Impact In this case, the oracle can return unexpected value if the _period is too small.

Suggestion Set a minimum limit on the _period in the function initialize and setPeriod.

3.2.8 Potential Issue 9: Non-Refundable Dust Tokens

Description In contract UniswapPoolHelper, the function zapWETH() is designed to help the user convert WETH tokens to LP tokens. It will invoke the function addLiquidityWETHOnly() to add liquidity in the pool for LP tokens. In this process, there may exist dust tokens which should be returned back to users. However, the UniswapPoolHelper doesn’t implement such functionality to handle these dust tokens.

function zapWETH(uint256 amount)
    public
    returns (uint256 liquidity)
{
    IWETH WETH = IWETH(wethAddr);
    WETH.transferFrom(msg.sender, address(liquidityZap), amount);
    liquidity = liquidityZap.addLiquidityWETHOnly(amount, address(this));
    IERC20 lp = IERC20(lpTokenAddr);
    
    liquidity = lp.balanceOf(address(this));
    lp.safeTransfer(msg.sender, liquidity);
}

Listing 3.11: UniswapPoolHelper.sol

Impact The dust tokens will remain in the contract, which can be extracted by others via the functio zapTokens(0,0).

Suggestion Implement the function to return dust tokens after adding liquidity.

3.2.9 Potential Issue 10: Improper Implementation of \_transfer() (II)

Description In contract IncentivizedERC20, the function _transfer() will invoke the function handle_ActionAfter() to update the status of the user in the contract ChefIncentivesController accordingly. However, the parameter senderBalance will not be updated if the sender equals the recipient, which is incorrect.

function _transfer(
        address sender,
        address recipient,
        uint256 amount
      ) internal virtual {
        require(sender != address(0), 'ERC20: transfer from the zero address');
        require(recipient != address(0), 'ERC20: transfer to the zero address');
    
        _beforeTokenTransfer(sender, recipient, amount);
    
        uint256 senderBalance = _balances[sender].sub(amount, 'ERC20: transfer amount exceeds balance');
    
        if (address(_getIncentivesController()) != address(0)) {
          // uint256 currentTotalSupply = _totalSupply;
          _getIncentivesController().handleActionBefore(sender);
          if (sender != recipient) {
            _getIncentivesController().handleActionBefore(recipient);
          }
        }
    
        _balances[sender] = senderBalance;
        uint256 recipientBalance = _balances[recipient].add(amount);
        _balances[recipient] = recipientBalance;
    
        if (address(_getIncentivesController()) != address(0)) {
          uint256 currentTotalSupply = _totalSupply;
          _getIncentivesController().handleActionAfter(sender, senderBalance, currentTotalSupply);
          if (sender != recipient) {
            _getIncentivesController().handleActionAfter(recipient, recipientBalance, currentTotalSupply);
          }
        }
      }

Listing 3.12: IncentivizedERC20.sol

Impact When users transfer to themselves, their state in contract ChefIncentivesController will not be updated properly, which will bring further issues for the rewards.

Suggestion Correct the senderBalance in the function handleActionAfter().

3.2.10 Potential Issue 11: Manipulatable Compound Rewards

Description In MFDPlus contract, the function _convertPendingRewardsToWeth() swaps the user’s rewards to WETH through the Uniswap router for relocking. However, there is no slippage check after the swapping.

IERC20(underlying).safeApprove(uniRouter, removedAmount);
    uint256[] memory amounts = IUniswapV2Router02(uniRouter)
    .swapExactTokensForTokens(
         removedAmount,
         0, // slippage handled after this function
         mfdHelper.getRewardToBaseRoute(underlying),
         address(this),
         block.timestamp + 10
     );

Listing 3.13: MFDPlus.sol

Impact The attacker can front-run the transaction to manipulate the price and gain the profit.

Suggestion Add the slippage check in function claimCompound().

3.2.11 Potential Issue 12: Lack of Access Control in setLeverager()

Description Function setLeverager() in the contract LendingPool has no access control.

uint256[] memory amounts = IUniswapV2Router02(uniRouter)
    .swapExactTokensForTokens(
         removedAmount,
         0, // slippage handled after this function
         mfdHelper.getRewardToBaseRoute(underlying),
         address(this),
         block.timestamp + 10
     );

Listing 3.14: LendingPool.sol

Impact If the leverager was not set at the beginning, an attacker could set the leverager to any address, thereby gaining control over the logic of the function depositWithAutoDLP().

Suggestion Set the leverager in the function initialize() or add the access control for function setLeverager().

3.2.12 Potential Issue 13: No Slippage Check in addLiquidityWETHOnly()

Description The user can use either borrowed WETH tokens (or his/her own ETH tokens) or vesting RDNT tokens in MFD contracts to get LP tokens (i.e., WETH-RDNT).

However, when adding the liquidity to the pool, the calculation of the required tokens is based on the amount of reserves in the pool, which can be manipulated. In this case, if the user only has WETH tokens, the function addLiquidityWETHOnly() will be invoked to swap half of the WETH tokens to RDNT tokens in the unbalanced pool without checking slippage.

function addLiquidityWETHOnly(uint256 _amount, address payable to)
    public
    returns (uint256 liquidity)
{
    require(to != address(0), "LiquidityZAP: Invalid address");
    uint256 buyAmount = _amount.div(2);
    require(buyAmount > 0, "LiquidityZAP: Insufficient ETH amount");

    (uint256 reserveWeth, uint256 reserveTokens) = getPairReserves();
    uint256 outTokens = UniswapV2Library.getAmountOut(
        buyAmount,
        reserveWeth,
        reserveTokens
    );

    _WETH.transfer(_tokenWETHPair, buyAmount);

    (address token0, address token1) = UniswapV2Library.sortTokens(
        address(_WETH),
        _token
    );
    IUniswapV2Pair(_tokenWETHPair).swap(
        _token == token0 ? outTokens : 0,
        _token == token1 ? outTokens : 0,
        address(this),
        ""
    );

    return _addLiquidity(outTokens, buyAmount, to);
}

Listing 3.15: LiquidityZap.sol

function getAmountOut(uint amountIn, uint reserveIn, uint reserveOut) internal pure returns (uint amountOut) {
       require(amountIn > 0, 'UniswapV2Library: INSUFFICIENT_INPUT_AMOUNT');
       require(reserveIn > 0 && reserveOut > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY');
       uint amountInWithFee = amountIn.mul(997);
       uint numerator = amountInWithFee.mul(reserveOut);
       uint denominator = reserveIn.mul(1000).add(amountInWithFee);
       amountOut = numerator / denominator;
   }  

Listing 3.16: UniswapV2Library.sol

Impact The attacker can front-run the transaction to manipulate the price and gain the profit.

Suggestion Check slippage in the function addLiquidityWETHOnly() or ensure it can only be invoked by UniswapPoolHelper.

3.2.13 Potential Issue 14: Lack of Check of borrowRatio in loopETH()

Description Function loopETH() is used for leverage borrowing and receives a parameter borrowRatio to specify the borrow ratio. However, the borrowRatio is not checked before the loop starts.

function loopETH(
        uint256 interestRateMode,
        uint256 borrowRatio,
        uint256 loopCount
    ) external payable {
        uint16 referralCode = 0;
        uint256 amount = msg.value;
        if (IERC20(address(weth)).allowance(address(this), address(lendingPool)) == 0) {
            IERC20(address(weth)).safeApprove(address(lendingPool), type(uint256).max);
        }
        if (IERC20(address(weth)).allowance(address(this), address(treasury)) == 0) {
            IERC20(address(weth)).safeApprove(treasury, type(uint256).max);
        }

        uint256 fee = amount.mul(feePercent).div(RATIO_DIVISOR);
        _safeTransferETH(treasury, fee);
        
        amount = amount.sub(fee);

        weth.deposit{value: amount}();
        lendingPool.deposit(address(weth), amount, msg.sender, referralCode);

        for (uint256 i = 0; i < loopCount; i += 1) {
            amount = amount.mul(borrowRatio).div(RATIO_DIVISOR);
            lendingPool.borrow(address(weth), amount, interestRateMode, referralCode, msg.sender);
            weth.withdraw(amount);

            fee = amount.mul(feePercent).div(RATIO_DIVISOR);
            _safeTransferETH(treasury, fee);

            weth.deposit{value: amount.sub(fee)}();
            lendingPool.deposit(address(weth), amount.sub(fee), msg.sender, referralCode);
        }

        zapWETHWithBorrow(wethToZap(msg.sender), msg.sender);
    }

Listing 3.17: Leverager.sol

Impact The borrowRatio may be higher than RATIO_DIVISOR which is inconsistent with the original design.

Suggestion Make sure that borrowRatio is less or equal to RATIO_DIVISOR.

3.2.14 Potential Issue 15: Lack of Check of Length between assets and poolIDs in setPoolIDs()

Description The function setPoolIDs() allows the owner to set different poolIDs for different assets. However, the lengths of these two arrays are not checked to be equal.

// Set pool ids of assets
    function setPoolIDs(address[] memory assets, uint256[] memory poolIDs) external onlyOwner {
        for (uint256 i = 0; i < assets.length; i += 1) {
            poolIdPerChain[assets[i]] = poolIDs[i];
        }
        emit PoolIDsUpdated(assets, poolIDs);
    } 

Listing 3.18: StarBorrow.sol

Impact The assets will not be assigned to correct poolIDs.

Suggestion Make sure the lengths of assets and poolIDs are equal.

3.2.15 Potential Issue 16: Lack of mint Privilege Revoke in addBountyContract()

Description Function addBountyContract() is used to set the new BountyManager. However, the original bounty contract still holds the mint privilege, which is against the original design.

function addBountyContract(address _bounty) external onlyOwner {
       BountyManager = _bounty;
       minters[_bounty] = true;
   }

Listing 3.19: Leverager.sol

Impact The deprecated BountyManager still has mint privileges.

Suggestion Revoke the mint privilege of origin BountyManager contract.

Feedback The function addBountyContract will only be called once to initialize the BountyManager.

3.2.16 Potential Issue 17: Minters Can Only be Assigned Once

Description The minters is used to record those who have the permission to access the function mint() and addReward(). However, when one of the minters (e.g., the contract ChefIncentivesController) is updated, the outdated minters can not be removed.

function setMinters(address[] memory _minters) external onlyOwner {
        require(!mintersAreSet);
        for (uint256 i; i < _minters.length; i++) {
            minters[_minters[i]] = true;
        }
        mintersAreSet = true;
    }

Listing 3.20: MultiFeeDistribution.sol

Impact The outdated minters can not be removed when they are upgraded.

Suggestion Implement a privileged function to modify minters.

Feedback Because the BountyManager, ChefIncentivesController and MultiFeeDistribution will be upgradable, so minters always keep the same proxy address.

3.3 Additional Recommendation

3.3.1 Potential Issue 18: Gas Optimization (zapVestingToLp() in Mfd)

Description The function zapVestingToLp() can only be invoked by the contract LockZap to transfer the locked earning of the user out. It iterates the earnings array of the user starting from the index 0, and checks whether the unlockTime is larger than the current timestamp. If so, this earning will be removed from the array and transferred out. However, since the unlockTime in the array is increasing with the index, it will be more efficient to start the iteration from the end of array to the beginning. If the unlockTime is smaller than the current timestamp, the loop can be broken.

function zapVestingToLp(address _user)
        external
        override
        returns (uint256 zapped)
    {
        require(msg.sender == lockZap);

        LockedBalance[] storage earnings = userEarnings[_user];
        uint256 length = earnings.length;

        for (uint256 i = 0; i < length; ) {
            // only vesting, so only look at currently locked items
            if (earnings[i].unlockTime > block.timestamp) {
                zapped = zapped.add(earnings[i].amount);
                // remove + shift array size
                earnings[i] = earnings[earnings.length - 1];
                earnings.pop();
                length = length.sub(1);
            } else {
                i = i.add(1);
            }
        }

        rdntToken.safeTransfer(lockZap, zapped);

        Balances storage bal = balances[_user];
        bal.earned = bal.earned.sub(zapped);
        bal.total = bal.total.sub(zapped);

        return zapped;
    }

Listing 3.21: MultiFeeDistribution.sol

Suggestion Start the iteration from the end of earnings to the beginning. If the unlockTime is smaller than the current timestamp, the loop can be broken.

3.3.2 Potential Issue 19: Non-empty Bounty Reserve in BountyManager

Description In function _sendBounty(), if there are not enough RDNT tokens for the transfer in the contract BountyManager, the event BountyReseveEmpty() will be emitted, and the contract will be paused. However, it’s possible that there are still some RDNT tokens left, which is inconsistent with the emitted event.

function _sendBounty(address _to, uint256 _amount)
		internal
		returns (uint256)
	{
		if (_amount == 0) {
			return 0;
		}

		uint256 bountyReserve = IERC20(rdnt).balanceOf(address(this));
		if(_amount > bountyReserve) {
			emit BountyReserveEmpty(bountyReserve);
			_pause();
		} else {
			IERC20(rdnt).safeTransfer(address(mfd), _amount);
			IMFDPlus(mfd).mint(_to, _amount, true);
			return _amount;
		}
	}

Listing 3.22: BountyManager.sol

Suggestion Transfer the left RDNT tokens out even if it’s not enough.

3.3.3 Potential Issue 20: Inconsistent Naming in requiredUsdValue()

Description The function requiredUsdValue() is used to check the required locked value of the user who wants to be qualified to earn rewards by holding RTokens. The calculation is based on the collateral value of the user, which is returned from the function getUserAccountData(). However, the returned value is named as totalCollateralETH, which is inconsistent with that in the function requiredUsdValue() (i.e., totalCollateralUSD).

Suggestion Standardize the naming conventions of functions with the right token name. For example, rename requiredUsdValue() to requiredEthValue().

Feedback We’d rather keep the AAVE contracts as similar as possible so we didn’t update the name.

3.4 Notes

3.4.1 Potential Issue 21: Depreciated MFDPlus

Description The contract MFDPlus is no longer used. The logic of compounding is moved into the contract AutoCompounder and other logic is moved into the contract MiddleFeeDistribution.

4. Appendix

4.1 Automated Static Security Testing Results

Table 4.1: Automated Static Security Testing Results. Found indidates the number of issues reported by the tools. FP means the number of false positives after our manual verification.

4.2 Automated Dynamic Security Testing Results

Table 4.2: Tested Properties for Lending related Logic

Table 4.3: Tested Properties for Staking related Logic

Table 4.4: Tested Properties for Other Features

5 Notices and Remarks

5.1 Disclaimer

This report does not constitute investment advice or a personal recommendation. It does not consider, and should not be interpreted as considering or having any bearing on, the potential economics of a token, token sale or any other product, service or other asset. Any entity should not rely on this report in any way, including for the purpose of making any decisions to buy or sell any token, product, service or other asset.

This report is not an endorsement of any particular project or team, and the report does not guarantee the security of any particular project. This security testing does not give any warranties on discovering all security issues of the smart contracts, i.e., the evaluation result does not guarantee the nonexistence of any further findings of security issues. As the security testing cannot be considered comprehensive, we always recommend proceeding with independent audits and a public bug bounty program to ensure the security of smart contracts.

The scope of this security testing is limited to the code mentioned in Section 1.2. Unless explicitly specified, the security of the language itself (e.g., the solidity language), the underlying compiling toolchain and the computing infrastructure are out of the scope.

5.2 Procedure of Auditing

We perform the audit according to the following procedure.

• Vulnerability DetectionWe first scan smart contracts with automatic code analyzers, and then manually verify (reject or confirm) the issues reported by them.

• Semantic AnalysisWe study the business logic of smart contracts and conduct further investigation on the possible vulnerabilities using an automatic fuzzing tool (developed by our research team). We also manually analyze possible attack scenarios with independent auditors to cross-check the result.

• RecommendationWe provide some useful advice to developers from the perspective of good programming practice, including gas optimization, code style, and etc.

We show the main concrete checkpoints in the following.

5.2.1 Software Security

• Reentrancy

• DoS

• Access control

• Data handling and data flow

• Exception handling

• Untrusted external call and control flow

• Initialization consistency

• Events operation

• Error-prone randomness

• Improper use of the proxy system

5.2.2 DeFi Security

• Semantic consistency

• Functionality consistency

• Permission management

• Business logic

• Token operation

• Emergency mechanism

• Oracle security

• Whitelist and blacklist

• Economic impact

• Batch transfer

5.2.3 NFT Security

• Duplicated item

• Verification of the token receiver

• Off-chain metadata security

5.2.4 Additional Recommendation

• Gas optimization

• Code quality and style

Note: The previous checkpoints are the main ones. We may use more checkpoints during the auditing process according to the functionality of the project.