Security Testing Report for Radiant V2
Mar 23 2023

Report Manifest

Item Description
Client Radiant Capital
Target Radiant V2

Version History

Version Date Description
1.0 March 15, 2023 First Version
2.0 March 21, 2023 Second Version

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

Information Description
Type Smart Contract
Language Solidity
Approach Static analysis, dynamic analysis, Semi-automatic and manual verification

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.

Table 1.1: Vulnerability Severity Classification

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

ID Severity Description Category Status
1 Medium No Reserved Interface for Resetting Function Pointers Software Security Fixed
2 Medium Improper Calculation of the Oracle DeFi Security Fixed
3 High Potential Drain of Funds through BaseBounty DeFi Security Fixed
4 Low Potential Invalid Emission Schedules DeFi Security Fixed
5 Low Skippable Emission schedules DeFi Security Confirmed
6 Medium Changeable Exchange Rate during Migration DeFi Security Fixed
7 High Improper Implementation of _transfer() (I) DeFi Security Fixed
8 Low Lack of Check on Period in UniV2TwapOracle DeFi Security Fixed
9 Medium Non-Refundable Dust Tokens DeFi Security Fixed
10 Medium Improper Implementation of _transfer() (II) DeFi Security Fixed
11 Medium Manipulatable Compound Rewards DeFi Security Fixed
12 Medium Lack of Access Control in setLeverager() DeFi Security Fixed
13 Medium No Slippage Check in addLiquidityWETHOnly() DeFi Security Confirmed
14 Low Lack of Check of borrowRatio in loopETH() DeFi Security Fixed
15 Low Lack of Check of Length between assets and poolIDs in setPoolIDs() DeFi Security Fixed
16 Low Lack of mint Privilege Revoke in addBountyContract() DeFi Security Confirmed
17 Low Minters Can Only be Assigned Once DeFi Security Confirmed
18 - Gas Optimization (zapVestingToLp() in Mfd) Recommendation Fixed
19 - Non-empty Bounty Reserve in BountyManager Recommendation Fixed
20 - Inconsistent Naming in requiredUsdValue() Recommendation Confirmed
21 - Depreciated MFDPlus Note Note Confirmed

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

Item Description
Severity Medium
Status Fixed in Version 7
Introduced by Version 1

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

Item Description
Severity Medium
Status Fixed in Version 11
Introduced by Version 1 and Version 4

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

Item Description
Severity High
Status Fixed in Version 4
Introduced by Version 1

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

Item Description
Severity Low
Status Fixed in Version 10
Introduced by Version 1

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

Item Description
Severity Low
Status Confirmed
Introduced by Version 1

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

Item Description
Severity Medium
Status Fixed in Version 5
Introduced by Version 1

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)

Item Description
Severity High
Status Fixed in Version 7
Introduced by Version 1

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

Item Description
Severity Low
Status Fixed in Version 9
Introduced by Version 1

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

Item Description
Severity Medium
Status Fixed in Version 5
Introduced by Version 1

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)

Item Description
Severity Medium
Status Fixed in Version 9
Introduced by Version 7

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

Item Description
Severity Medium
Status Fixed in Version 10
Introduced by Version 5

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()

Item Description
Severity Medium
Status Fixed in Version 9
Introduced by Version 1

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()

Item Description
Severity Medium
Status Confirmed
Introduced by Version 1

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()

Item Description
Severity Low
Status Fixed in Version 10
Introduced by Version 1

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()

Item Description
Severity Low
Status Fixed in Version 10
Introduced by Version 1

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()

Item Description
Severity Low
Status Confirmed
Introduced by Version 1

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

Item Description
Severity Low
Status Confirmed
Introduced by Version 1

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)

Item Description
Status Fixed in Version 10
Introduced by Version 1

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

Item Description
Status Fixed in Version 10
Introduced by Version 1

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()

Item Description
Status Confirmed
Introduced by Version 1

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

Item Description
Status Confirmed
Introduced by version 10

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.

ID Detector Description Impact Found FP Result
1 arbitrary-send-erc20 Calling transferFrom with arbitrary from High 1 1 Passed
2 array-by-reference Modifying storage array by value High 0 0 Passed
3 incorrect-shift Incorrect order of parameters in a shift instruction High 0 0 Passed
4 multiple-constructors Multiple constructor schemes High 0 0 Passed
5 name-reused Reusing contract’s name High 0 0 Passed
6 protected-vars Modifying variables directly without access control High 0 0 Passed
7 rtlo Using Right-To-Left-Override control character High 0 0 Passed
8 shadowing-state State variables shadowing High 1 1 Passed
9 suicidal Functions allowing anyone to destruct the contract High 0 0 Passed
10 uninitialized-state Uninitialized state variables High 3 3 Passed
11 uninitialized-storage Uninitialized storage variables High 0 0 Passed
12 unprotected-upgrade Unprotected upgradeable contract High 1 1 Passed
13 arbitrary-send-erc20-permit transferFrom uses arbitrary from with permit High 0 0 Passed
14 arbitrary-send-eth Functions that send Ether to arbitrary destinations High 0 0 Passed
15 controlled-array-length Tainted array length assignment High 0 0 Passed
16 controlled-delegatecall Controlled delegatecall destination High 0 0 Passed
17 delegatecall-loop Payable functions using delegatecall inside a loop High 0 0 Passed
18 msg-value-loop Using msg.value inside a loop High 0 0 Passed
19 reentrancy-eth Reentrancy vulnerabilities (theft of ethers) High 5 5 Passed
20 storage-array Signed storage integer array compiler bug High 0 0 Passed
21 unchecked-transfer Unchecked tokens transfer High 12 12 Passed
22 weak-prng Weak PRNG High 0 0 Passed
23 domain-separator-collision Detects ERC20 tokens that have a function whose signature collides with EIP-2612’s DOMAIN_SEPARATOR() Medium 0 0 Passed
24 enum-conversion Detects dangerous enum conversion Medium 0 0 Passed
25 erc20-interface Incorrect ERC20 interfaces Medium 0 0 Passed
26 erc721-interface Incorrect ERC721 interfaces Medium 0 0 Passed
27 incorrect-equality Dangerous strict equalities Medium 23 23 Passed
28 locked-ether Contracts that lock ether Medium 1 1 Passed
29 mapping-deletion Deletion on mapping containing a structure Medium 0 0 Passed
30 shadowing-abstract State variables shadowing from abstract contracts Medium 0 0 Passed
31 tautology Tautology or contradiction Medium 0 0 Passed
32 write-after-write Unused write Medium 3 3 Passed
33 boolean-cst Misuse of Boolean constant Medium 0 0 Passed
34 constant-function-asm Constant functions using assembly code Medium 0 0 Passed
35 constant-function-state Constant functions changing the state Medium 0 0 Passed
36 divide-before-multiply Imprecise arithmetic operations order Medium 20 20 Passed
37 reentrancy-no-eth Reentrancy vulnerabilities (no theft of ethers) Medium 12 12 Passed
38 reused-constructor Reused base constructor Medium 0 0 Passed
39 tx-origin Dangerous usage of tx.origin Medium 1 1 Passed
40 unchecked-lowlevel Unchecked low-level calls Medium 0 0 Passed
41 unchecked-send Unchecked send Medium 0 0 Passed
42 uninitialized-local Uninitialized local variables Medium 33 33 Passed
43 unused-return Unused return values Medium 19 19 Passed

4.2 Automated Dynamic Security Testing Results

Table 4.2: Tested Properties for Lending related Logic

ID Property Result
1 Calling deposit never leads to a decrease of onBehalfOf’s RToken amount Passed
2 Calling withdraw never leads to an increase of msg.sender’s RToken amount Passed
3 Calling borrow with stable interest rate mode never leads to a decrease of onBehalfOf’s StableDebtToken. Passed
4 Calling borrow with variable interest rate mode never leads to a decrease of onBehalfOf’s VariableDebtToken. Passed
5 Calling borrow with onBehalfOf that does not equal to msg.sender never leads to an increase of msg.sender’s borrow allowance. Passed
6 Calling repay with stable interest rate mode never leads to an increase of onBehalfOf’s StableDebtToken. Passed
7 Calling repay with variable interest rate mode never leads to an increase of onBehalfOf’s VariableDebtToken. Passed
8 liquidityIndex will never decrease. Passed
9 liquidityIndex will remain constant within the same block. Passed
10 variableBorrowIndex will never decrease. Passed
11 variableBorrowIndex will remain constant within the same block. Passed
12 Decreasing collateral amounts will never lead to health factor less than 1. Passed
13 Increasing borrowing amounts will never lead to health factor less than 1. Passed

Table 4.3: Tested Properties for Staking related Logic

ID Property Result
1 User’s total balance always equals the sum of locked balance, unlocked balance and earned balance. Passed
2 User’s locked balance always equals the sum of userLocks amount Passed
3 User’s lockedWithMultiplier balance always equals the sum of userLocks amount times userLocks multiplier Passed
4 lockedSupply always equals the sum of users’ locked balance Passed
5 lockedSupplyWithMultiplier always equals the sum of users’ lockedWithMultiplier balance Passed
6 rewardPerTokenStored never decreases. Passed
7 rewardPerTokenStored will remain constant within the same block. Passed
8 totalSupply always equals the sum of users’ amount Passed
9 accRewardPerShare never decreases. Passed
10 accRewardPerShare will remain constant within the same block. Passed

Table 4.4: Tested Properties for Other Features

ID Property Result
1 WETH and RDNT balance of the contract LockedZap will always be zero. Passed
2 WETH and RDNT balance of the contract LiquidityZap will always be zero. Passed
3 WETH and RDNT balance of the contract BalancerPoolHelper will always be zero. Passed
4 WETH and RDNT balance of the contract UniswapPoolHelper will always be zero. Passed
5 Calling loop will always lead to user eligible for rewards Passed
6 Calling loopETH will always lead to user eligible for rewards Passed
7 Calling executeBounty with _execute equals false will never lead to storage change. Passed
8 Calling transfer with sender equals to receiver never leads to balance change. Failed in Version 1. Passed in Version 7

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.

Sign up for the latest updates
#10: ThirdWeb Incident: Incompatibility Between Trusted Modules Exposes Vulnerability
Security Insights

#10: ThirdWeb Incident: Incompatibility Between Trusted Modules Exposes Vulnerability

This blog shows the vulnerability and attack caused by Incompatibility of commonly used modules.

#9: MEV Bot 0xd61492: From Predator to Prey in an Ingenious Exploit
Security Insights

#9: MEV Bot 0xd61492: From Predator to Prey in an Ingenious Exploit

On August 3, 2023, an MEV Bot on Arbitrum was attacked, resulting in $800K in loss. The root cause of this attack was **Insufficient User Input Verification**.

#8: SushiSwap Incident: A Clumsy Rescue Attempt Leads to a Series of Copycat Attacks
Case Studies

#8: SushiSwap Incident: A Clumsy Rescue Attempt Leads to a Series of Copycat Attacks

On April 9, 2023, SushiSwap became the target of an exploit due to an Unverified External Parameter. The total loss is about $3.3 million.

BlockSec uses cookies and other identifiers to analyze our traffic in accordance. We also share information about your use of our site with our analytics partners. By remaining on this website, you consent to our use of cookies and the Privacy Policy.