Date: 14.11.2023-08.12.2023

Platform: Cantina

Findings summary Link to heading

Table of Contents Link to heading

Medium Findings Link to heading

[M-01] ERC20WrapperBundler calls are missing return value checks Link to heading

Description: Link to heading

The Morpho Blue bundler offers users the capability to bundle calls, including interactions with contracts that implement the ERC20Wrapper interface. This functionality is facilitated through the erc20WrapperDepositFor() and erc20WrapperWithdrawTo() functions.

The current implementation presents a vulnerability as it neglects to check the boolean return value of the underlying ERC20Wrapper functions. While the OpenZeppelin implementation returns true for successful calls and reverts on errors, alternative implementations may simply return false in case of an unsuccessful call, allowing the call to proceed without reverting.

Given that the documentation specifies the assumption that the wrapper implements the ERC20Wrapper interface without explicitly detailing the OpenZeppelin functionality, variations in implementation are valid. Consequently, this issue may result in a scenario where a user sends tokens for wrapping to the contract, but the wrapper fails to transfer them out of the bundler (e.g., due to a blocklist) and returns false. Since the return value is not checked, the execution continues, leaving the user’s tokens within the bundler and vulnerable to theft.

Impact Link to heading

This issue can lead to a potential loss of user funds as a user will expect the bundler to revert in the case of any of the transferring functions failing, but instead, the bundler will finish the execution. leaving tokens in the bundler. These tokens could then be stolen by anyone.

POC Link to heading

To simulate this issue I have implemented a simple ERC20Wrapper that returns false on an incorrect deposit instead of reverting.

import {IERC20} from “../../lib/openzeppelin-contracts/contracts/token/ERC20/ERC20.sol”;

contract ERC20WrapperNotReverting {
    IERC20 private immutable _underlying;
    mapping(address => uint256) public balances;

    constructor(IERC20 underlyingToken) {
        _underlying = underlyingToken;
    }

    function underlying() public view returns (IERC20) {
        return _underlying;
    }

    /**
     * @dev Allow a user to deposit underlying tokens and mint the corresponding number of wrapped tokens.
     */
    function depositFor(address account, uint256 amount) public virtual returns (bool) {
        if(msg.sender != address(0x123456))
        {
            //simulating only dedicated users being allowed to call the function
            return false;
        }

        _underlying.transferFrom(msg.sender, address(this), amount);
        balances[account] += amount;
        return true;
    }
}

In the following POC one can see the issue happen:

function testErc20WrapperDepositForNoRevert() public {
    //Deploy the non reverting mock
    ERC20WrapperNotReverting wrapper2 = new ERC20WrapperNotReverting(loanToken);

    bundle.push(_erc20WrapperDepositFor(address(wrapper2), 100));

    loanToken.setBalance(address(bundler), 100);

    vm.prank(RECEIVER);
    bundler.multicall(bundle);

    //Tokens are still left in the contract but it didn't revert
    assertEq(loanToken.balanceOf(address(bundler)), 100, "loan.balanceOf(bundler)");
    assertEq(loanWrapper.balanceOf(RECEIVER), 0, "loanWrapper.balanceOf(RECEIVER)");
}

The testcase can be run by:

1. Adding the ERC20WrapperNotReverting contract to the morpho-blue-bundlers/src/mocks folder
2. Importing it using import {ERC20WrapperNotReverting} from “../../src/mocks/ERC20WrapperNotReverting.sol”;
3. Adding the testcase to morpho-blue-bundlers/test/forge/ERC20WrapperBundlerBundlerLocalTest
4. Run it using forge test -vvvv –match-test “testErc20WrapperDepositForNoRevert”

Recommendation: Link to heading

Mitigate this issue by incorporating return value checks for the calls to the functions ERC20Wrapper(wrapper).depositFor() and ERC20Wrapper(wrapper).withdrawTo(). The recommended adjustments are as follows:

Deposit:

bool success = ERC20Wrapper(wrapper).depositFor(initiator(), amount);
require(success, "Deposit was unsuccessful");

Withdraw:

bool success = ERC20Wrapper(wrapper).withdrawTo(account, amount);
require(success, "Withdraw was unsuccessful");

[M-02] Incorrect Debt Repayment in AaveV3 Caused by Passing type(uint256).max Link to heading

Description: Link to heading

In the Morpho bundler, users can leverage a feature to bundle calls to AAVE V3 pools, notably through the aaveV3Repay() function, designed for repaying a user’s debt on an Aave V3 pool. Users can specify the repayment amount using the amount parameter, with a distinctive functionality activated when a user passes type(uint256).max, as stated in the documentation:

@param amount The amount of `asset` to repay. Pass `type(uint256).max` to repay the bundler's `asset` balance.

However, an issue arises in the implementation of this functionality due to an error in the conditional statement:

if (amount != type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));

Regrettably, when a user passes type(uint256).max, the amount is directly sent to AAVE_V3_POOL.repay(), ignoring the bundler’s balance. Examining the Aave V3 pool contract reveals that Aave handles the type(uint256).max case differently, as described in this comment:

* @param amount The amount to repay
* - Send the value type(uint256).max in order to repay the whole debt for `asset` on the specific `debtMode`

Aave attempts to recover the entire debt from the bundler’s balance, resulting in a revert if paybackAmount > token.balanceOf(bundler). In cases where paybackAmount < token.balanceOf(bundler), tokens are left inside the bundler, susceptible to theft by anyone.

Impact: Link to heading

This vulnerability poses a risk of potential user fund loss when paybackAmount < token.balanceOf(bundler). Users, adhering to the aaveV3Repay() documentation, may construct their multicall with the assumption that the bundler uses all remaining funds for repayment. However, the bundler retains the difference, leaving it inside and open to theft by any entity calling the bundler.

POC Link to heading

To illustrate the potential issue, consider the following scenario:

1. A user initiates a series of bundled calls, culminating in a call to aaveV3Repay() with the parameter type(uint256).max. The objective is to use the 10 remaining tokens, after the execution of prior calls, to settle a debt.
2. The call is directed to Aave, indicating type(uint256).max to express the intent of utilizing the entire remaining balance in the bundler. The function snippet is outlined below:

function aaveV3Repay(address asset, uint256 amount, uint256 interestRateMode) external payable protected {
	if (amount != type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	
	_approveMaxTo(asset, address(AAVE_V3_POOL));
	
	AAVE_V3_POOL.repay(asset, amount, interestRateMode, initiator());
}

3. Aave interprets the type(uint256).max parameter, calculating the total outstanding debt the user has (e.g., 5 tokens).
4. Aave proceeds to withdraw the 5 tokens from the bundler using the transferFrom() method.
5. The multicall execution concludes.
6. Contrary to the user’s intention of full repayment, 5 tokens persist within the contract, posing a potential vulnerability for unauthorized entities to appropriate them.

Recommendation: Link to heading

To align with the intended functionality, adjustments are needed in the implementation of aaveV3Repay(), specifically by adapting the conditional statement around the reduction of the amount. A refined example is provided below:

function aaveV3Repay(address asset, uint256 amount, uint256 interestRateMode) external payable protected {
	if (amount == type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	
	_approveMaxTo(asset, address(AAVE_V2_POOL));
	
	AAVE_V3_POOL.repay(asset, amount, interestRateMode, initiator());
}

[M-03] Incorrect Debt Repayment in AaveV2 due to type(uint256).max Parameter Link to heading

Description: Link to heading

Within the Morpho bundler, a feature enables users to bundle calls to AAVE V2 pools. Specifically, the aaveV2Repay() function is designed for repaying a user’s debt on an Aave V2 pool. This function allows users to specify the repayment amount using the amount parameter. An interesting functionality is triggered when a user passes type(uint256).max as the amount, as indicated in the documentation snippet:

@param amount The amount of `asset` to repay. Pass `type(uint256).max` to repay the bundler's `asset` balance.

However, the corresponding implementation of this functionality has an error in the following conditional statement:

if (amount != type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));

Unfortunately, in the case where a user passes type(uint256).max, the amount is directly forwarded to AAVE_V2_POOL.repay(), without utilizing the balance of the bundler. A closer examination of the Aave V2 pool contract reveals that Aave handles the type(uint256).max case differently, as described in the following comment found here:

* @param amount The amount to repay
* - Send the value type(uint256).max in order to repay the whole debt for `asset` on the specific `debtMode`

Aave attempts to recover the entire debt from the bundler’s balance, leading to a revert if paybackAmount > token.balanceOf(bundler). If paybackAmount < token.balanceOf(bundler) tokens will be left inside the bundler, resulting in anyone being able to steal them.

Impact: Link to heading

This vulnerability introduces the risk of potential user fund loss when paybackAmount < token.balanceOf(bundler). Users, following the aaveV2Repay() documentation, will construct their multicall under the assumption that the bundler utilizes all remaining funds for the repayment. Contrary to this expectation, the bundler retains the difference, leaving it inside. Subsequently, these leftover funds become susceptible to theft by any entity calling the bundler and transferring them out.

POC Link to heading

An illustrative proof of concept can be outlined through the following steps:

1. The user compiles multiple calls, concluding with a call to aaveV2Repay() utilizing type(uint256).max. The intention is to utilize the remaining 10 tokens, after the execution of the preceding calls, to repay a debt.
2. The call is directed to Aave, specifying type(uint256).max to signify the desire to use the entire remaining balance in the bundler. The function code is as follows:

function aaveV2Repay(address asset, uint256 amount, uint256 interestRateMode) external payable protected {
	if (amount != type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	
	_approveMaxTo(asset, address(AAVE_V2_POOL));
	
	AAVE_V2_POOL.repay(asset, amount, interestRateMode, initiator());
}

3. Aave recognizes the type(uint256).max and calculates the total debt the user still has (e.g., 5 tokens).
4. Aave withdraws the 5 tokens from the bundler using the transferFrom() method.
5. The multicall execution is completed.
6. Despite the intended repayment, 5 tokens remain within the contract, presenting an opportunity for unauthorized individuals to seize them.

Recommendation: Link to heading

To align with the intended functionality, the implementation of aaveV2Repay() requires adjustment, specifically the removal of the conditional statement surrounding the reduction of the amount. An improved example is provided below:

function aaveV2Repay(address asset, uint256 amount, uint256 interestRateMode) external payable protected {
	if (amount == type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	
	_approveMaxTo(asset, address(AAVE_V2_POOL));
	
	AAVE_V2_POOL.repay(asset, amount, interestRateMode, initiator());
}

[M-04] Flawed Debt Repayment in AaveV3Optimizer Link to heading

Description: Link to heading

The Morpho bundler facilitates the bundling of calls to AAVE V3 pools, including the aaveV3OptimizerRepay() function designed for repaying a user’s debt on an Aave V3 pool. Users can specify the repayment amount using the amount parameter, and a feature is activated when users pass type(uint256).max as the amount, as highlighted in the documentation snippet:

/// @param amount The amount of `underlying` to repay. Pass `type(uint256).max` to repay the bundler's `underlying`
/// balance.

However, an error exists in the corresponding implementation of this functionality within the following conditional statement:

if (amount != type(uint256).max) amount = Math.min(amount, ERC20(underlying).balanceOf(address(this)));

Regrettably, when a user passes type(uint256).max, the amount is directly forwarded to AAVE_V3_OPTIMIZER.repay(), bypassing the utilization of the bundler’s balance. A closer examination of the Aave V3 pool contract reveals that Aave handles the type(uint256).max case differently, as described in the following comment found here:

* @param amount The amount to repay
* - Send the value type(uint256).max in order to repay the whole debt for `asset` on the specific `debtMode`

Aave attempts to recover the entire debt from the bundler’s balance, resulting in a revert if paybackAmount > token.balanceOf(bundler). If paybackAmount < token.balanceOf(bundler), tokens are left inside the bundler, becoming susceptible to theft by any entity calling the bundler and transferring them out.

Impact: Link to heading

This vulnerability poses the risk of potential user fund loss when paybackAmount < token.balanceOf(bundler). Users, following the aaveV3OptimizerRepay() documentation, may construct their multicall under the assumption that the bundler utilizes all remaining funds for the repayment. Contrary to this expectation, the bundler retains the difference, leaving it inside and vulnerable to theft by any entity calling the bundler and transferring out the leftover funds.

POC Link to heading

An exemplary proof can be described in the following steps.

1. The user initiates a bundle of multiple calls, concluding with a call to aaveV3OptimizerRepay() with type(uint256).max to utilize the remaining 10 tokens after the previous calls to repay a debt.
2. The call with type(uint256).max is directed to Aave through the following function:

function aaveV3OptimizerRepay(address underlying, uint256 amount) external payable protected {
	if (amount != type(uint256).max) amount = Math.min(amount, ERC20(underlying).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	  
	_approveMaxTo(underlying, address(AAVE_V3_OPTIMIZER));
	
	AAVE_V3_OPTIMIZER.repay(underlying, amount, initiator());
}

3. Aave recognizes the type(uint256).max and calculates the total debt the user still has (5 tokens).
4. Aave withdraws the 5 tokens from the bundler using transferFrom().
5. The multicall execution concludes.
6. Despite the intended use, 5 tokens remain within the contract and are susceptible to being stolen by any entity.

Recommendation: Link to heading

To align with the intended functionality, the implementation of aaveV3OptimizerRepay() requires adjustment, specifically the removal of the conditional statement surrounding the reduction of the amount. An improved example is provided below:

function aaveV3OptimizerRepay(address asset, uint256 amount, uint256 interestRateMode) external payable protected {
	if (amount == type(uint256).max) amount = Math.min(amount, ERC20(asset).balanceOf(address(this)));
	
	require(amount != 0, ErrorsLib.ZERO_AMOUNT);
	
	_approveMaxTo(asset, address(AAVE_V2_POOL));
	
	AAVE_V3_POOL.repay(asset, amount, interestRateMode, initiator());
}