Core Contracts

Summary

The LendingPool's withdrawal process contains three flaws in the asset flow when withdrawing from the Curve vault. When users attempt to withdraw their assets, the process fails due to:

1. Incorrect ownership parameter: The _withdrawFromVault() function uses msg.sender as the owner of vault shares, when it should be using address(this) (LendingPool), which is the actual owner of the shares from the deposit process.

2. Incorrect asset recipient: The withdrawn assets are sent to the LendingPool contract instead of the RToken contract, which is responsible for transferring the assets to the user.

3. Zero loss tolerance: The maxLoss parameter is hardcoded to 0, which means any vault losses will cause withdrawals to fail completely, potentially leading to permanent fund lockup [https://docs.yearn.fi/developers/v3/Integrating_v3#maxloss]

Vulnerability Details

Pre-Condition:

1. The issue in the _depositIntoVault function has been fixed (see PoC below) and described in issue: "Incorrect Asset Transfer Implementation in LendingPool's Vault Deposit Function Leads To Transaction Failure".

2. The LendingPool contractLendingPool::setCurveVault() was called to set the vault contract address

3. User called LendingPool::deposit() to deposit crvUSD into the Pool, which calls _depositIntoVault() => this mints the shares of the vault to the LendingPool

The issue occurs in the following sequence:

1. A user calls withdraw() in the LendingPool contract

2. The _ensureLiquidity() function is called to ensure sufficient liquidity

3. If needed, _withdrawFromVault() is called to withdraw assets from the Curve vault

4. The withdrawn assets are sent to the LendingPool contract (address(this))

5. The RToken contract attempts to transfer the assets during the burn() function to the user but fails with ERC20InsufficientBalance because it doesn't have the balance

6. The second issue here is, the LendingPool contract is the owner of the vault shares, the _withdrawFromVault() function should use address(this) as argument for the Owner of the shares, and not msg.sender

7. The third issue is that the maxLoss parameter is hardcoded to 0. If the Vault made any losses this will cause another transaction failure.

Key problematic code:

When deposits are made via _depositIntoVault(), the LendingPool contract (address(this)) becomes the owner of the vault shares:

The Lender wants to withdraw now and calls LendingPool::withdraw() which calls the internal function _ensureLiquidity(). This calls _withdrawFromVault() :

The assets should instead be sent to the RToken contract which handles the actual transfer to the user:

PoC

⚠️ In order to test the withdraw flow we first need to fix the deposit flow which requires a code change in the LendingPool::_depositIntoVault() function:

In order to run the test you need to:

1. Run foundryup to get the latest version of Foundry

2. Install hardhat-foundry: npm install --save-dev @nomicfoundation/hardhat-foundry

3. Import it in your Hardhat config: require("@nomicfoundation/hardhat-foundry");

4. Make sure you've set the BASE_RPC_URL in the .env file or comment out the forking option in the hardhat config.

5. Run npx hardhat init-foundry

6. There is one file in the test folder that will throw an error during compilation so rename the file in test/unit/libraries/ReserveLibraryMock.sol to => ReserveLibraryMock.sol_broken so it doesn't get compiled anymore (we don't need it anyways).

7. Create a new folder test/foundry

8. Paste the below code into a new test file i.e.: FoundryTest.t.sol

9. Run the test: forge test --mc FoundryTest -vvvv

Impact

• Users are unable to withdraw their assets from the protocol because the transaction will always revert => Loss of funds for users

• This effectively locks user funds in the crvUSDVault => The owner of the shares is the LendingPool contract, there is no way to redeem / withdraw from the pool anymore.

• According to Yearn's v3 documentation, setting maxLoss to 0 means:

  • The withdrawal will revert if there is any loss in the vault

  • Users cannot withdraw their funds even if they would accept some loss

  • Even when there is 0.01% loss in the vault, withdrawals won't work

• Protocol needs to redeploy everything

Tools Used

• Foundry

• Manual Review

Recommendations

Modify the arguments in the _withdrawFromVault() function, to fix issue 1 & 2:

To fix the issue 3 regarding the maxLoss the protocol could:

• Let the user pass in an aditional argument for the maxLoss he is willing to take when he calls LendingPool::withdraw() and handle the burn / transfer of the correct token amounts and update all necessary states accordingly