Function Pool.validateOffer() does not work correctly in case principalAmount > currentBalance

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L411

Vulnerability details

Impact

In Pool contract, undeployed funds could be deposited to Aave or Lido to earn base yield. When an offer of Pool is accepted from MultiSourceLoan, the function validateOffer() is called to validate the terms and also to pull the undeployed funds back in case the contract balance is insufficient.

if (principalAmount > undeployedAssets) {
    revert InsufficientAssetsError();
} else if (principalAmount > currentBalance) {
    IBaseInterestAllocator(getBaseInterestAllocator).reallocate(
        currentBalance, principalAmount - currentBalance, true // @audit Incorrect
    );
}

However, the input params of reallocate() are incorrect, resulting in the contract balance might still be insufficient for the loan after calling the function.

Proof of Concept

Consider the scenario

1. Assume we have currentBalance = 500, baseRateBalance = 1000 usdc and principalAmount = 700 usdc.
2. Since the current contract balance is insufficient (500 < 700), reallocate() will be called with input

reallocate(currentBalance, principalAmount - currentBalance, true)
reallocate(500, 200, true)

3. In function reallocate() shown in the code snippet below, we can see that in case _currentIdle > _targetIdle, the contract even deposits more funds to AavePool instead of withdrawing.

function reallocate(uint256 _currentIdle, uint256 _targetIdle, bool) external {
    address pool = _onlyPool();
    if (_currentIdle > _targetIdle) {
        uint256 delta = _currentIdle - _targetIdle;
        ERC20(_usdc).transferFrom(pool, address(this), delta);
        IAaveLendingPool(_aavePool).deposit(_usdc, delta, address(this), 0);
    } else {
        uint256 delta = _targetIdle - _currentIdle;
        IAaveLendingPool(_aavePool).withdraw(_usdc, delta, address(this));
        ERC20(_usdc).transfer(pool, delta);
    }

    emit Reallocated(_currentIdle, _targetIdle);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Call reallocate(0, principalAmount - currentBalance, true) instead.

Assessed type

Other

[c4-judge]

0xA5DF marked the issue as primary issue

[0xA5DF]

IIUC the impact is DoS that occurs only under certain conditions, in that case I think severity should be medium

[0xend]

Agree on issue -I think it's med (not high).

[c4-judge]

0xA5DF changed the severity to 2 (Med Risk)

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xA5DF]

Marking as med, see my comment above

[0xend]

https://github.com/pixeldaogg/florida-contracts/pull/365

changed to reallocate(currentBalance, principalAmount, true) instead of proposed solution (same result) to be compliant with the interface