multicall() doesn't correctly enforce borrowATokenCap

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/Multicall.sol#L29-L42

Vulnerability details

Impact

The Size protocol enforces a borrowATokenCap, which is a limit on how large the totalSupply() of the protocol's borrowAToken can become. This is useful for self-limiting the TVL of the protocol in a "guarded launch" approach.

To enforce this cap, the Deposit.sol contract has the following code:

// borrow aToken cap is not validated in multicall,
//   since users must be able to deposit more tokens to repay debt
if (!state.data.isMulticall) {
    state.validateBorrowATokenCap();
}

Note that the implementation of validateBorrowATokenCap() is the following:

function validateBorrowATokenCap(State storage state) external view {
    if (state.data.borrowAToken.totalSupply() > state.riskConfig.borrowATokenCap) {
        revert Errors.BORROW_ATOKEN_CAP_EXCEEDED(
            state.riskConfig.borrowATokenCap, state.data.borrowAToken.totalSupply()
        );
    }
}

As specified in the comments of the first code snippet, this validation is skipped when isMulticall == true, since users may want to deposit to reduce their debt (e.g. using the compensate() logic). So, in the multicall() function, the following logic facilitates its own borrowATokenCap as follows:

uint256 borrowATokenSupplyBefore = state.data.borrowAToken.balanceOf(address(this));
uint256 debtTokenSupplyBefore = state.data.debtToken.totalSupply();


results = new bytes[](data.length);
for (uint256 i = 0; i < data.length; i++) {
    results[i] = Address.functionDelegateCall(address(this), data[i]);
}


uint256 borrowATokenSupplyAfter = state.data.borrowAToken.balanceOf(address(this));
uint256 debtTokenSupplyAfter = state.data.debtToken.totalSupply();


state.validateBorrowATokenIncreaseLteDebtTokenDecrease(
    borrowATokenSupplyBefore, debtTokenSupplyBefore, borrowATokenSupplyAfter, debtTokenSupplyAfter
);

Notice that this code is tracking the increase in borrowAToken supply by using balanceOf(address(this)). This is incorrect. The borrowAToken.balanceOf(address(this)) value represents the Size contract's own borrowAToken amount, which is simply the amount of unclaimed repayments (which can be reduced arbitrarily since claim() is permissionless). As a result, the multicall() logic does not correctly limit the borrowAToken supply, making the entire cap obsolete.

Proof of Concept

For a PoC, add the following test to DepositValidation.t.sol. This test shows that the exact same deposit can fail in a single deposit() but succeed in multicall():

function test_Deposit_validation_borrowATokenCap_multicall_bug() public {
    uint256 amount = 4_000_000;
    _mint(address(weth), alice, amount * 1e18);
    _mint(address(usdc), alice, amount * 1e6);
    _approve(alice, address(weth), address(size), amount * 1e18);
    _approve(alice, address(usdc), address(size), amount * 1e6);

    DepositParams memory dp = DepositParams({token: address(usdc), amount: amount * 1e6, to: alice});

    // Notice that this call reverts
    vm.startPrank(alice);
    vm.expectRevert(
        abi.encodeWithSelector(
            Errors.BORROW_ATOKEN_CAP_EXCEEDED.selector, size.riskConfig().borrowATokenCap, amount * 1e6
        )
    );
    size.deposit(dp);

    // But doing the exact same action in multicall succeeds
    bytes[] memory data = new bytes[](1);
    data[0] = abi.encodeCall(size.deposit, dp);
    size.multicall(data);
}

Tools Used

Manual analysis.

Recommended Mitigation Steps

In the multicall() function, change borrowAToken.balanceOf(address(this)) to borrowAToken.totalSupply().

Assessed type

Invalid Validation

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as duplicate of #238