Size uses wrong source to query available liquidity on Aave, resulting in borrow and lend operations being bricked upon mainnet deployment

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/CapsLibrary.sol#L67-L71

Vulnerability details

Bug Description

Size queries the underlyingBorrowToken balance of the variablePool during borrowing and lending actions in order to check available liquidity. If there is not enough available liquidity for the new borrower to withdraw the borrowed assets, the function call will revert:

Size::buyCreditMarket

178:    function buyCreditMarket(BuyCreditMarketParams calldata params) external payable override(ISize) whenNotPaused {
...
184:        state.validateVariablePoolHasEnoughLiquidity(amount); // @audit: liquidity check

Size::sellCreditMarket

188:    function sellCreditMarket(SellCreditMarketParams memory params) external payable override(ISize) whenNotPaused {
...
194:        state.validateVariablePoolHasEnoughLiquidity(amount); // @audit: liquidity check

CapsLibrary::validateVariablePoolHasEnoughLiquidity

67:    function validateVariablePoolHasEnoughLiquidity(State storage state, uint256 amount) public view {
68:        uint256 liquidity = state.data.underlyingBorrowToken.balanceOf(address(state.data.variablePool)); // @audit: ATokens hold liquidity, not variablePool
69:        if (liquidity < amount) {
70:            revert Errors.NOT_ENOUGH_BORROW_ATOKEN_LIQUIDITY(liquidity, amount);
71:        }

However, Aave actual holds all liquidity in the AToken contracts, not the Pool contracts. Therefore, the liquidity check above will always fail when the variablePool references a live instance of an actual Aave Pool. This is due to the fact that the variablePool balance will be 0 (does not hold any liquidity), causing borrowing and lending actions to revert on line 70 in CapsLibrary.sol.

The below code snippets are taken from out of scope contracts, but are shown to further explain why the test suite did not detect this vulnerability:

SupplyLogic::executeSupply

52:  function executeSupply(
...
67:    IERC20(params.asset).safeTransferFrom(msg.sender, reserveCache.aTokenAddress, params.amount);

As shown above, the actual implementation of the Aave pool will transfer supplied assets to the aTokenAddress on supplies. The Aave pool implementation will then transfer the assets from the AToken to the recipient during withdraws, since the liquidity is stored in the AToken contract:

SupplyLogic::executeWithdraw

106:  function executeWithdraw(
...
139:    IAToken(reserveCache.aTokenAddress).burn(
140:      msg.sender,
141:      params.to,
142:      amountToWithdraw,
143:      reserveCache.nextLiquidityIndex
144:    );

AToken::burn

96:   function burn(
97:     address from,
98:     address receiverOfUnderlying,
99:     uint256 amount,
100:    uint256 index
101:  ) external virtual override onlyPool {
102:    _burnScaled(from, receiverOfUnderlying, amount, index);
103:    if (receiverOfUnderlying != address(this)) {
104:      IERC20(_underlyingAsset).safeTransfer(receiverOfUnderlying, amount);

However, Size uses a PoolMock contract for their test suite, which implements logic that differs from actual Aave Pool contracts:

PoolMock.sol#L67-L78

67:    function supply(address asset, uint256 amount, address onBehalfOf, uint16) external {
68:        Data memory data = datas[asset];
69:        IERC20Metadata(asset).transferFrom(msg.sender, address(this), amount); // @audit: underlying transferred from supplier to PoolMock contract
70:        data.aToken.mint(address(this), onBehalfOf, amount, data.reserveIndex);
71:    }
72:
73:    function withdraw(address asset, uint256 amount, address to) external returns (uint256) {
74:        Data memory data = datas[asset];
75:        data.aToken.burn(msg.sender, address(data.aToken), amount, data.reserveIndex); // @audit: no assets transferred from AToken
76:        IERC20Metadata(asset).safeTransfer(to, amount); // @audit: underlying transferred from PoolMock contract to supplier
77:        return amount;
78:    }

As shown above, the PoolMock contract transfers all supplied assets to the PoolMock contract itself, not the AToken contract. This differs from how actual Aave Pools handle liquidity on live networks.

Impact

Borrow (sellCreditMarket) and lend (buyCreditMarket) operations will be bricked when Size is deployed and integrates with Aave on a live network. This report is labeled as High due to the fact that the main functionalities of this lending protocol will be unusable upon deployment.

Proof of Concept

The following PoC simply showcases the fact that the AToken does not hold the liquidity in the test suite. See here to observe that ATokens are the contracts that hold liquidity for Aave in live networks.

Modify the file below and run with forge test --mc BuyCreditLimitTest --mt test_BuyCreditLimit_buyCreditLimit_adds_loanOffer_to_orderbook:

diff --git a/./test/local/actions/BuyCreditLimit.t.sol b/./test/local/actions/BuyCreditLimit.t.sol
index a8c6317..66dd63e 100644
--- a/./test/local/actions/BuyCreditLimit.t.sol
+++ b/./test/local/actions/BuyCreditLimit.t.sol
@@ -23,6 +23,9 @@ contract BuyCreditLimitTest is BaseTest {
         assertTrue(_state().alice.user.loanOffer.isNull());
         _buyCreditLimit(alice, block.timestamp + 12 days, YieldCurveHelper.pointCurve(12 days, 1.01e18));
         assertTrue(!_state().alice.user.loanOffer.isNull());
+        address aToken = variablePool.getReserveData(address(usdc)).aTokenAddress;
+        assertEq(usdc.balanceOf(address(variablePool)), 100e6);
+        assertEq(usdc.balanceOf(aToken), 0);
     }

Tools Used

manual

Recommended Mitigation

When checking available liquidity on Aave, Size should query the underlyingBorrowToken balance of the AToken instead of the variablePool:

diff --git a/./src/libraries/CapsLibrary.sol b/./src/libraries/CapsLibrary.sol
index 7e35e90..b7ce7a1 100644
--- a/./src/libraries/CapsLibrary.sol
+++ b/./src/libraries/CapsLibrary.sol
@@ -65,9 +65,11 @@ library CapsLibrary {
     /// @param state The state struct
     /// @param amount The amount of cash to withdraw
     function validateVariablePoolHasEnoughLiquidity(State storage state, uint256 amount) public view {
-        uint256 liquidity = state.data.underlyingBorrowToken.balanceOf(address(state.data.variablePool));
+        address aToken = state.data.variablePool.getReserveData(address(state.data.underlyingBorrowToken)).aTokenAddress;
+        uint256 liquidity = state.data.underlyingBorrowToken.balanceOf(aToken);
         if (liquidity < amount) {
             revert Errors.NOT_ENOUGH_BORROW_ATOKEN_LIQUIDITY(liquidity, amount);
         }
     }
 }

Assessed type

Other

[aviggiano]

Fixed in SizeCredit/size-solidity#127

[c4-judge]

hansfriese marked the issue as satisfactory

[c4-judge]

hansfriese marked the issue as selected for report

[samuraii77]

Hi, the severity of this issue should be a medium. Please take a look at what a high severity issue should be:

Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

This issue does not cause such an issue. In fact, it can be completely mitigated by redeploying the contract or upgrading the contract (if it is upgradeable, I am not sure if it was) whenever the issue is noticed which would be extremely quickly as it would occur on the first deposit of USDC essentially causing nothing else than some wasted time and headache.

[hansfriese]

I agree. Medium is more appropriate as there is no direct fund loss.

[c4-judge]

hansfriese changed the severity to 2 (Med Risk)