GrowthDeFi WHEAT

Description

The test suite at this stage is not complete. For complicated systems such as GrowthDefi, which uses many different modules and interacts with different DeFi protocols, it is crucial to have a full test coverage (could be verified with solidity-coverage tool) that includes both unit tests and integration tests to cover both the correctness of the core logic and possible interface issues. As we’ve seen in some smart contract incidents, a complete test suite can prevent issues that might be hard to find with manual reviews.

Description

Across the code base, especially in the strategy token contracts, there are highly similar code regions. Duplication of such code may become an issue in future development as fixes may be applied only partially by accident, resulting in inconsistent system behaviour and hard-to-find bugs.

Examples

In the strategy token contracts:

• calcSharesFromAmount and other view functions
• deposit and withdraw checks and control flow
• gulp conversions from reward- to reserve-tokens
• recoverLostFunds business logic

Recommendation

It is recommended to deduplicate the codebase by inheriting generic contracts that provide common control flow and business logic actions. Then, by overriding local methods, implementing the concrete strategy token behaviour specific to each third-party interaction.

This will reduce the potential for future bugs and increase the codebase’s maintainability and extensibility.

Description

There are few possible attack vectors by the owner:

1. All strategies have fees from rewards. In addition to that, the PancakeSwap strategy has deposit fees. The default deposit fees equal zero; the maximum is limited to 5%:

   wheat-v1-core-audit/contracts/PancakeSwapCompoundingStrategyToken.sol:L29-L33

   uint256 constant MAXIMUM_DEPOSIT_FEE = 5e16; // 5%
   uint256 constant DEFAULT_DEPOSIT_FEE = 0e16; // 0%
   
   uint256 constant MAXIMUM_PERFORMANCE_FEE = 50e16; // 50%
   uint256 constant DEFAULT_PERFORMANCE_FEE = 10e16; // 10%
   

   When a user deposits tokens, expecting to have zero deposit fees, the owner can frontrun the deposit and increase fees to 5%. If the deposit size is big enough, that may be a significant amount of money.

2. In the gulp function, the reward tokens are exchanged for the reserve tokens on the exchange:

   wheat-v1-core-audit/contracts/PancakeSwapCompoundingStrategyToken.sol:L218-L244

   function gulp(uint256 _minRewardAmount) external onlyEOAorWhitelist nonReentrant
   {
   	uint256 _pendingReward = _getPendingReward();
   	if (_pendingReward > 0) {
   		_withdraw(0);
   	}
   	{
   		uint256 _totalReward = Transfers._getBalance(rewardToken);
   		uint256 _feeReward = _totalReward.mul(performanceFee) / 1e18;
   		Transfers._pushFunds(rewardToken, collector, _feeReward);
   	}
   	if (rewardToken != routingToken) {
   		require(exchange != address(0), "exchange not set");
   		uint256 _totalReward = Transfers._getBalance(rewardToken);
   		Transfers._approveFunds(rewardToken, exchange, _totalReward);
   		IExchange(exchange).convertFundsFromInput(rewardToken, routingToken, _totalReward, 1);
   	}
   	if (routingToken != reserveToken) {
   		require(exchange != address(0), "exchange not set");
   		uint256 _totalRouting = Transfers._getBalance(routingToken);
   		Transfers._approveFunds(routingToken, exchange, _totalRouting);
   		IExchange(exchange).joinPoolFromInput(reserveToken, routingToken, _totalRouting, 1);
   	}
   	uint256 _totalBalance = Transfers._getBalance(reserveToken);
   	require(_totalBalance >= _minRewardAmount, "high slippage");
   	_deposit(_totalBalance);
   }
   

   The owner can change the exchange parameter to the malicious address that steals tokens. The owner then calls gulp with _minRewardAmount==0, and all the rewards will be stolen. The same attack can be implemented in fee collectors and the buyback contract.

Recommendation

Use a timelock to avoid instant changes of the parameters.

Description

When a new deposit is happening, the current pending rewards are not withdrawn and re-invested yet. And they are not taken into account when calculating the number of shares that the depositor receives. The number of shares is calculated as if there were no pending rewards. The other side of this issue is that all the withdrawals are also happening without considering the pending rewards. So currently, it makes more sense to withdraw right after gulp to gather the rewards. In addition to the general “unfairness” of the reward distribution during the deposit/withdrawal, there is also an attack vector created by this issue.

The Attack

If the deposit is made right before the gulp function is called, the rewards from the gulp are distributed evenly across all the current deposits, including the ones that were just recently made. So if the deposit-gulp-withdraw sequence is executed, the caller receives guaranteed profit. If the attacker also can execute these functions briefly (in one block or transaction) and take a huge loan to deposit a lot of tokens, almost all the rewards from the gulp will be stolen by the attacker. The easy 1-transaction attack with a flashloan can be done by the owner, miner, whitelisted contracts, or any contract if the onlyEOAorWhitelist modifier is disabled or stops working (https://github.com/ConsenSys/growthdefi-audit-2021-06/issues/3). Even if onlyEOAorWhitelist is working properly, anyone can take a regular loan to make the attack. The risk is not that big because no price manipulation is required. The price will likely remain the same during the attack (few blocks maximum).

Recommendation

If issue issue 6.3 is fixed while allowing anyone call the gulp contract, the best solution would be to include the gulp call at the beginning of the deposit and withdraw. In case of withdrawing, there should also be an option to avoid calling gulp as the emergency case.

Description

Each strategy token contract provides a gulp method to fetch pending rewards, convert them into the reserve token and split up the balances. One share is sent to the fee collector as a performance fee, while the rest is deposited into the respective MasterChef contract to accumulate more rewards. Suboptimal trades are prevented by passing a minimum slippage value with the function call, which results in revert if the expected reserve token amount cannot be provided by the trade(s).

The slippage parameter and the trades performed in gulp open the function up to proactive sandwich attacks. The slippage parameter can be freely set by the attacker, resulting in the system performing arbitrarily bad trades based on how much the attacker can manipulate the liquidity of involved assets around the gulp function call.

This attack vector is significant under the following assumptions:

• The exchange the trade is performed on allows significant changes in liquidity pools in a single transaction (e.g., not limiting transactions to X% of the pool amount),
• The attacker can frontrun legitimate gulp calls with reasonable slippage values,
• Trades are performed, i.e. when rewardToken != routingToken and/or routingToken != reserveToken hold true.

Examples

This affects the gulp functions in all the strategies:

• PancakeSwapCompoundingStrategyToken
• AutoFarmCompoundingStrategyToken
• PantherSwapCompoundingStrategyToken

and also fees collectors and the buyback adapters:

• PantherSwapBuybackAdapter
• AutoFarmFeeCollectorAdapter
• PancakeSwapFeeCollector
• UniversalBuyback

Recommendation

There are different possible solutions to this issue and all have some tradeoffs. Initially, we came up with the following suggestion:

• The onlyOwner modifier should be added to the gulp function to ensure only authorized parties with reasonable slippages can execute trades on behalf of the strategy contracts. Furthermore, additional slippage checks can be added to avoid unwanted behavior of authorized addresses, e.g., to avoid a bot setting unreasonable slippage values due to a software bug.

But in order to fix another issue (https://github.com/ConsenSys/growthdefi-audit-2021-06/issues/8), we came up with the alternative solution:

• Use oracles to restrict users from calling the gulp function with unreasonable slippage (more than 5% from the oracle’s moving average price). The side effect of that solution is that sometimes the outdated price will be used. That means that when the price crashes, nobody will be able to call the gulp.

Description

Every withdraw function in the strategy contracts is calculating the expected amount of the returned tokens before withdrawing them:

wheat-v1-core-audit/contracts/PantherSwapCompoundingStrategyToken.sol:L200-L208

function withdraw(uint256 _shares, uint256 _minAmount) external onlyEOAorWhitelist nonReentrant
{
	address _from = msg.sender;
	(uint256 _amount, uint256 _withdrawalAmount, uint256 _netAmount) = _calcAmountFromShares(_shares);
	require(_netAmount >= _minAmount, "high slippage");
	_burn(_from, _shares);
	_withdraw(_amount);
	Transfers._pushFunds(reserveToken, _from, _withdrawalAmount);
}

After that, the contract is trying to transfer this pre-calculated amount to the msg.sender. It is never checked whether the intended amount was actually transferred to the strategy contract. If the amount is lower, that may result in reverting the withdraw function all the time and locking up tokens.

Even though we did not find any specific case of returning a different amount of tokens, it is still a good idea to handle this situation to minimize relying on the security of the external contracts.

Recommendation

There are a few options how to mitigate the issue:

• Double-check the balance difference before and after the MasterChef’s withdraw function is called.
• Handle this situation in the emergency mode (https://github.com/ConsenSys/growthdefi-audit-2021-06/issues/11).

Description

All the underlying MasterChef contracts have the emergency withdrawal mode, which allows simpler withdrawal (excluding the rewards):

    // Withdraw without caring about rewards. EMERGENCY ONLY.
    function emergencyWithdraw(uint256 _pid) public nonReentrant {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
        uint256 amount = user.amount;
        user.amount = 0;
        user.rewardDebt = 0;
        user.rewardLockedUp = 0;
        user.nextHarvestUntil = 0;
        pool.lpToken.safeTransfer(address(msg.sender), amount);
        emit EmergencyWithdraw(msg.sender, _pid, amount);
    }

    // Withdraw without caring about rewards. EMERGENCY ONLY.
    function emergencyWithdraw(uint256 _pid) public {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];
        pool.lpToken.safeTransfer(address(msg.sender), user.amount);
        emit EmergencyWithdraw(msg.sender, _pid, user.amount);
        user.amount = 0;
        user.rewardDebt = 0;
    }

    // Withdraw without caring about rewards. EMERGENCY ONLY.
    function emergencyWithdraw(uint256 _pid) public nonReentrant {
        PoolInfo storage pool = poolInfo[_pid];
        UserInfo storage user = userInfo[_pid][msg.sender];

        uint256 wantLockedTotal =
            IStrategy(poolInfo[_pid].strat).wantLockedTotal();
        uint256 sharesTotal = IStrategy(poolInfo[_pid].strat).sharesTotal();
        uint256 amount = user.shares.mul(wantLockedTotal).div(sharesTotal);

        IStrategy(poolInfo[_pid].strat).withdraw(msg.sender, amount);

        pool.want.safeTransfer(address(msg.sender), amount);
        emit EmergencyWithdraw(msg.sender, _pid, amount);
        user.shares = 0;
        user.rewardDebt = 0;
    }

While it’s hard to predict how and why the emergency mode can be enabled in the underlying MasterChef contracts, these functions are there for a reason, and it’s safer to be able to use them. If some emergency happens and this is the only way to withdraw funds, the funds in the strategy contracts will be locked forever.

Recommendation

Add the emergency mode implementation.

Description

Panther token has a cap in transfer sizes, so any transfer in the contract is limited beforehand:

wheat-v1-core-audit/contracts/PantherSwapCompoundingStrategyToken.sol:L218-L245

function gulp(uint256 _minRewardAmount) external onlyEOAorWhitelist nonReentrant
{
	uint256 _pendingReward = _getPendingReward();
	if (_pendingReward > 0) {
		_withdraw(0);
	}
	uint256 __totalReward = Transfers._getBalance(rewardToken);
	(uint256 _feeReward, uint256 _retainedReward) = _capFeeAmount(__totalReward.mul(performanceFee) / 1e18);
	Transfers._pushFunds(rewardToken, buyback, _feeReward);
	if (rewardToken != routingToken) {
		require(exchange != address(0), "exchange not set");
		uint256 _totalReward = Transfers._getBalance(rewardToken);
		_totalReward = _capTransferAmount(rewardToken, _totalReward, _retainedReward);
		Transfers._approveFunds(rewardToken, exchange, _totalReward);
		IExchange(exchange).convertFundsFromInput(rewardToken, routingToken, _totalReward, 1);
	}
	if (routingToken != reserveToken) {
		require(exchange != address(0), "exchange not set");
		uint256 _totalRouting = Transfers._getBalance(routingToken);
		_totalRouting = _capTransferAmount(routingToken, _totalRouting, _retainedReward);
		Transfers._approveFunds(routingToken, exchange, _totalRouting);
		IExchange(exchange).joinPoolFromInput(reserveToken, routingToken, _totalRouting, 1);
	}
	uint256 _totalBalance = Transfers._getBalance(reserveToken);
	_totalBalance = _capTransferAmount(reserveToken, _totalBalance, _retainedReward);
	require(_totalBalance >= _minRewardAmount, "high slippage");
	_deposit(_totalBalance);
}

Fees here are calculated from the full amount of rewards (__totalReward ):

wheat-v1-core-audit/contracts/PantherSwapCompoundingStrategyToken.sol:L225

(uint256 _feeReward, uint256 _retainedReward) = _capFeeAmount(__totalReward.mul(performanceFee) / 1e18);

But in fact, if the amount of the rewards is too big, it will be capped, and the residuals will be “taxed” again during the next call of the gulp function. That behavior leads to multiple taxations of the same tokens, which means increased fees.

Recommendation

The best solution would be to cap __totalReward first and then calculate fees from the capped value.

Description

Panther token has a limit on the transfer size. Because of that, all the Panther transfer values in the PantherSwapCompoundingStrategyToken are also capped beforehand. The following function is called to cap the size of fees:

wheat-v1-core-audit/contracts/PantherSwapCompoundingStrategyToken.sol:L357-L366

function _capFeeAmount(uint256 _amount) internal view returns (uint256 _capped, uint256 _retained)
{
	_retained = 0;
	uint256 _limit = _calcMaxRewardTransferAmount();
	if (_amount > _limit) {
		_amount = _limit;
		_retained = _amount.sub(_limit);
	}
	return (_amount, _retained);
}

This function should return the capped amount and the amount of retained tokens. But because the _amount is changed before calculating the _retained, the retained amount will always be 0.

Recommendation

Calculate the retained value before changing the amount.

Description

The gulp and pendingBurning functions of the UniversalBuyback contract use the hardcoded, constant values of DEFAULT_REWARD_BUYBACK1_SHARE and DEFAULT_REWARD_BUYBACK2_SHARE to determine the ratio the trade value is split with.

Consequently, any call to setRewardSplit to set a new ratio will be ineffective but still result in a ChangeRewardSplit event being emitted. This event can deceive system operators and users as it does not reflect the correct values of the contract.

Examples

wheat-v1-core-audit/contracts/UniversalBuyback.sol:L80-L81

uint256 _amount1 = _balance.mul(DEFAULT_REWARD_BUYBACK1_SHARE) / 1e18;
uint256 _amount2 = _balance.mul(DEFAULT_REWARD_BUYBACK2_SHARE) / 1e18;

wheat-v1-core-audit/contracts/UniversalBuyback.sol:L97-L98

uint256 _amount1 = _balance.mul(DEFAULT_REWARD_BUYBACK1_SHARE) / 1e18;
uint256 _amount2 = _balance.mul(DEFAULT_REWARD_BUYBACK2_SHARE) / 1e18;

Recommendation

Instead of the default values, rewardBuyback1Share and rewardBuyback2Share should be used.

Description

The onlyEOAorWhitelist modifier is used in various locations throughout the code. It performs a check that asserts the message sender being equal to the transaction origin to assert the calling party is not a smart contract.

This approach may stop working if EIP-3074 and its AUTH and AUTHCALL opcodes get deployed.

While the OpenZeppelin reentrancy guard does not depend on tx.origin, the EOA check does. Its evasion can result in additional attack vectors such as flash loans opening up. It is noteworthy that preventing smart contract interaction with the protocol may limit its opportunities as smart contracts cannot integrate with it in the same way that GrowthDeFi integrates with its third-party service providers.

The onlyEOAorWhitelist modifier may give a false sense of security because it won’t allow making a flash loan attack by most of the users. But the same attack can still be made by some people or with more risk:

• The owner and the whitelisted contracts are not affected by the modifier.

• The modifier can be disabled:

  **wheat-v1-core-audit/contracts/WhitelistGuard.sol:L21-L28**
  ```solidity
  modifier onlyEOAorWhitelist()
  {
  	if (enabled) {
  		address _from = _msgSender();
  		require(tx.origin == _from || whitelist.contains(_from), "access denied");
  	}
  	_;
  }
  ```
  

  And in the deployment script, this modifier is disabled for testing purposes, and it’s important not to forget to turn it in on the production:

  wheat-v1-core-audit/migrations/02_deploy_contracts.js:L50

  await pancakeSwapFeeCollector.setWhitelistEnabled(false); // allows testing
  

• The attack can usually be split into multiple transactions. Miners can put these transactions closely together and don’t take any additional risk. Regular users can take a risk, take the loan, and execute the attack in multiple transactions or even blocks.

Recommendation

It is strongly recommended to monitor the progress of this EIP and its potential implementation on the Binance Smart Chain. If this functionality gets enabled, the development team should update the contract system to use the new opcodes. We also strongly recommend relying less on the fact that only EOA will call the functions. It is better to write the code that can be called by the external smart contracts without compromising its security.

Description

The practice of pulling funds from a user (by using safeTransferFrom) and then later pushing (some) of the funds back to the user occurs in various places in the Exchange contract. In case one of the used token contracts (or one of its dependent calls) externally calls the Exchange owner, the owner may utilize that to call back Exchange.recoverLostFunds and drain (some) user funds.

Examples

wheat-v1-core-audit/contracts/Exchange.sol:L80-L89

function convertFundsFromInput(address _from, address _to, uint256 _inputAmount, uint256 _minOutputAmount) external override returns (uint256 _outputAmount)
{
	address _sender = msg.sender;
	Transfers._pullFunds(_from, _sender, _inputAmount);
	_inputAmount = Math._min(_inputAmount, Transfers._getBalance(_from)); // deals with potential transfer tax
	_outputAmount = UniswapV2ExchangeAbstraction._convertFundsFromInput(router, _from, _to, _inputAmount, _minOutputAmount);
	_outputAmount = Math._min(_outputAmount, Transfers._getBalance(_to)); // deals with potential transfer tax
	Transfers._pushFunds(_to, _sender, _outputAmount);
	return _outputAmount;
}

wheat-v1-core-audit/contracts/Exchange.sol:L121-L130

function joinPoolFromInput(address _pool, address _token, uint256 _inputAmount, uint256 _minOutputShares) external override returns (uint256 _outputShares)
{
	address _sender = msg.sender;
	Transfers._pullFunds(_token, _sender, _inputAmount);
	_inputAmount = Math._min(_inputAmount, Transfers._getBalance(_token)); // deals with potential transfer tax
	_outputShares = UniswapV2LiquidityPoolAbstraction._joinPoolFromInput(router, _pool, _token, _inputAmount, _minOutputShares);
	_outputShares = Math._min(_outputShares, Transfers._getBalance(_pool)); // deals with potential transfer tax
	Transfers._pushFunds(_pool, _sender, _outputShares);
	return _outputShares;
}

wheat-v1-core-audit/contracts/Exchange.sol:L99-L111

function convertFundsFromOutput(address _from, address _to, uint256 _outputAmount, uint256 _maxInputAmount) external override returns (uint256 _inputAmount)
{
	address _sender = msg.sender;
	Transfers._pullFunds(_from, _sender, _maxInputAmount);
	_maxInputAmount = Math._min(_maxInputAmount, Transfers._getBalance(_from)); // deals with potential transfer tax
	_inputAmount = UniswapV2ExchangeAbstraction._convertFundsFromOutput(router, _from, _to, _outputAmount, _maxInputAmount);
	uint256 _refundAmount = _maxInputAmount - _inputAmount;
	_refundAmount = Math._min(_refundAmount, Transfers._getBalance(_from)); // deals with potential transfer tax
	Transfers._pushFunds(_from, _sender, _refundAmount);
	_outputAmount = Math._min(_outputAmount, Transfers._getBalance(_to)); // deals with potential transfer tax
	Transfers._pushFunds(_to, _sender, _outputAmount);
	return _inputAmount;
}

wheat-v1-core-audit/contracts/Exchange.sol:L139-L143

function recoverLostFunds(address _token) external onlyOwner
{
	uint256 _balance = Transfers._getBalance(_token);
	Transfers._pushFunds(_token, treasury, _balance);
}

Recommendation

Reentrancy guard protection should be added to Exchange.convertFundsFromInput, Exchange.convertFundsFromOutput, Exchange.joinPoolFromInput, Exchange.recoverLostFunds at least, and in general to all public/external functions since gas price considerations are less relevant for contracts deployed on BSC.