In OrigamiAaveV3BorrowAndLend, the ability to recover borrowToken should be restricted

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x0e716c2445a7c8b5222878723462cb2f0d60afd3b7ee78a00138154ee6681bac
Severity: low

Description:
Description
In OrigamiAaveV3BorrowAndLend, there are two functions for recovering tokens.
One is the reclaimSurplusDebt function, used to recover borrowToken when there is no outstanding debt.
The other is recoverToken function, which allows recovery of any token.
This implies that borrowToken can be recovered even if there are still outstanding debts using the recoverToken function.

Attack Scenario
We are able to recover surplus borrowToken only when there are no outstanding debts.

function reclaimSurplusDebt(uint256 amount, address recipient) external override onlyPositionOwner {
    uint256 _bal = debtBalance();
    if (_bal != 0) revert CommonEventsAndErrors.InvalidAmount(borrowToken, _bal);
    IERC20(borrowToken).safeTransfer(recipient, amount);
    emit SurplusDebtReclaimed(amount, recipient);
}

Additionally, we have the capability to recover any token using the recoverToken function.

function recoverToken(address token, address to, uint256 amount) external onlyElevatedAccess {       
    // If the token to recover is the aaveAtoken, can only remove any *surplus* reserves (ie donation reserves).
    // It can't dip into the actual user added reserves. 
    if (token == address(aaveAToken)) {
        uint256 bal = aaveAToken.balanceOf(address(this));
        if (amount > (bal - suppliedBalance())) revert CommonEventsAndErrors.InvalidAmount(token, amount);
    }

    emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
    IERC20(token).safeTransfer(to, amount);
}

In this function, there is an additional check for the supplyToken, but there are no restrictions for borrowToken.
Thus, we can recover borrowToken even if there are still outstanding debts.

Of course, this function can only be invoked by elevated users.
However, to mitigate unexpected risks, it's essential to implement additional guardian checks.

For instance, in OrigamiAaveV3IdleStrategy, the recoverToken function can also be called by elevated users.
However, we've included the following check to prevent aTokens from being recovered:

function recoverToken(address token, address to, uint256 amount) external override onlyElevatedAccess {
    if (token == address(aToken)) revert CommonEventsAndErrors.InvalidToken(token);

    emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
    IERC20(token).safeTransfer(to, amount);
}

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

function recoverToken(address token, address to, uint256 amount) external onlyElevatedAccess {       
    // If the token to recover is the aaveAtoken, can only remove any *surplus* reserves (ie donation reserves).
    // It can't dip into the actual user added reserves. 
+     if (token == borrowToken) revert();

    if (token == address(aaveAToken)) {
        uint256 bal = aaveAToken.balanceOf(address(this));
        if (amount > (bal - suppliedBalance())) revert CommonEventsAndErrors.InvalidAmount(token, amount);
    }

    emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
    IERC20(token).safeTransfer(to, amount);
}

Or

function recoverToken(address token, address to, uint256 amount) external onlyElevatedAccess {       
    // If the token to recover is the aaveAtoken, can only remove any *surplus* reserves (ie donation reserves).
    // It can't dip into the actual user added reserves. 
+     if (token == borrowToken) {
+         uint256 _bal = debtBalance();
+         if (_bal != 0) revert CommonEventsAndErrors.InvalidAmount(borrowToken, _bal);
+     }

    if (token == address(aaveAToken)) {
        uint256 bal = aaveAToken.balanceOf(address(this));
        if (amount > (bal - suppliedBalance())) revert CommonEventsAndErrors.InvalidAmount(token, amount);
    }

    emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
    IERC20(token).safeTransfer(to, amount);
}

[FCSE507]

We are using the surplus borrowToken when there are outstanding debts.

function _rebalanceUpFlashLoanCallback(
    uint256 flashLoanAmount, 
    uint256 fee, 
    RebalanceUpParams memory params, 
    bool force
) internal {
    {
        uint256 surplusAfterSwap = _debtToken.balanceOf(address(this)) - flashRepayAmount;
        uint256 borrowLendSurplus = _debtToken.balanceOf(address(_borrowLend));  // @audit, here
        uint256 totalSurplus = borrowLendSurplus + surplusAfterSwap;
        if (totalSurplus > params.repaySurplusThreshold) {
            if (surplusAfterSwap != 0) {
                _debtToken.safeTransfer(address(_borrowLend), surplusAfterSwap); 
            }
            totalDebtRepaid += _borrowLend.repay(totalSurplus);  // @audit, here
        }
    }
}

[frontier159]

Thanks for raising.

• reclaimSurplusDebt() is only callable by positionOwner
• recoverToken() is admin only, only callable by elevatedAccess

The operational flow is:

1. positionOwner transfers borrowToken to OrigamiAaveV3BorrowAndLend
2. positionOwner calls OrigamiAaveV3BorrowAndLend::repay()
   1. This calls aavePool.repay() which pulls the tokens in

The OrigamiAaveV3BorrowAndLend should not ordinarily hold borrowToken as part of it's normal operations -- only when the debt is fully repaid in which case there's a surplus of borrowToken remaining. We want to allow positionOwner to reclaim these.

However - there may also be a case where positionOwner accidentally transfers borrowToken and either doesn't call repay() correctly, or calls it for a lesser amount.

In this case, we want elevatedAccess to be able to recover those tokens.

tldr we intentionally want to leave this as is.

[FCSE507]

@frontier159 , thanks for explanation.

I believe the positionOwner of OrigamiAaveV3BorrowAndLend is OrigamiLovTokenFlashAndBorrowManager because it interacts with OrigamiAaveV3BorrowAndLend and calls functions such as supply and withdraw, which typically can only be called by the positionOwner.
However, there's no functionality to call the reclaimSurplusDebt function in OrigamiLovTokenFlashAndBorrowManager. According to your explanation, the OrigamiLovTokenFlashAndBorrowManager should indeed have functionality to call this function?.

[frontier159]

According to your explanation, the OrigamiLovTokenFlashAndBorrowManager should indeed have functionality to call this function?.

It was added to future proof the OrigamiAaveV3BorrowAndLend contract for future integration. You can see we added a range of unit tests to cover the functionality
https://github.com/TempleDAO/origami-public/blob/185a93e25071b6a110ca190e94a6a826e982b2d6/apps/protocol/test/foundry/unit/common/borrowLend/OrigamiAaveV3BorrowAndLend.t.sol#L185

[FCSE507]

Yes, I've already reviewed this test file.
However, typically, this could be considered a missing functionality because auditors might not be aware of functions intended for future use, so their absence presently may be acceptable.
We depend on the documentation and code to identify any issues.
Based on my experience, this type of issue could be classified as medium severity, but it's up to you of course.

@frontier159 , I don't mind if you assign a low to this issue.

[frontier159]

Point taken that it can be taken as being incomplete. Will add a low.