hats-finance / Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd

GNU Affero General Public License v3.0
2 stars 0 forks source link

The donated debt token can be used for repayment when attempting to rebalance up in the OrigamiLovTokenFlashAndBorrowManager. #51

Open hats-bug-reporter[bot] opened 7 months ago

hats-bug-reporter[bot] commented 7 months ago

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

Description: Description\ There may be some donated debt tokens in the OrigamiLovTokenFlashAndBorrowManager. During the rebalance up process, we convert reserve tokens to debt tokens. If there are remaining debt tokens after paying off the flash loan, we use these tokens to repay the debt. However, in some cases, we can also use the donated debt tokens.

Attack Scenario\ There is a functionality to reclaim donated tokens, implying that there may occasionally be donated debt tokens available.

function recoverToken(address token, address to, uint256 amount) external override onlyElevatedAccess {
    emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
    IERC20(token).safeTransfer(to, amount);
}

During the rebalance up process, we first convert reserve tokens into debt tokens to pay off the flash loan. As indicated in the comment, if there are any extra debt tokens resulting from the swap, we use these tokens to repay the remaining debt.

function _rebalanceUpFlashLoanCallback(
    uint256 flashLoanAmount, 
    uint256 fee, 
    RebalanceUpParams memory params, 
    bool force
) internal {
    {
        uint256 debtTokenReceived = swapper.execute(_reserveToken, params.collateralToWithdraw, _debtToken, params.swapData);
        if (debtTokenReceived < flashRepayAmount) {
            revert CommonEventsAndErrors.Slippage(flashRepayAmount, debtTokenReceived);
        }
    }

    // If over the threshold, return any surplus [debtToken] from the swap to 
the borrowLend
    // And pay down residual debt
    {
        uint256 surplusAfterSwap = _debtToken.balanceOf(address(this)) - flashRepayAmount;
        uint256 borrowLendSurplus = _debtToken.balanceOf(address(_borrowLend));
        uint256 totalSurplus = borrowLendSurplus + surplusAfterSwap;
        if (totalSurplus > params.repaySurplusThreshold) {
            if (surplusAfterSwap != 0) {
                _debtToken.safeTransfer(address(_borrowLend), surplusAfterSwap);
            }
            totalDebtRepaid += _borrowLend.repay(totalSurplus);
        }
    }
}

However, _debtToken.balanceOf(address(this)) also includes the donated debt tokens.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

    The following fix is correct because it ensures that debtTokenReceived is greater than flashRepayAmount.

    function _rebalanceUpFlashLoanCallback(
    uint256 flashLoanAmount, 
    uint256 fee, 
    RebalanceUpParams memory params, 
    bool force
    ) internal {
    {
        uint256 debtTokenReceived = swapper.execute(_reserveToken, params.collateralToWithdraw, _debtToken, params.swapData);
        if (debtTokenReceived < flashRepayAmount) {
            revert CommonEventsAndErrors.Slippage(flashRepayAmount, debtTokenReceived);
        }
    }
    
    // If over the threshold, return any surplus [debtToken] from the swap to 
    the borrowLend
    // And pay down residual debt
    {
    -         uint256 surplusAfterSwap = _debtToken.balanceOf(address(this)) - flashRepayAmount;
    +         uint256 surplusAfterSwap = debtTokenReceived  - flashRepayAmount;
    
        uint256 borrowLendSurplus = _debtToken.balanceOf(address(_borrowLend));
        uint256 totalSurplus = borrowLendSurplus + surplusAfterSwap;
        if (totalSurplus > params.repaySurplusThreshold) {
            if (surplusAfterSwap != 0) {
                _debtToken.safeTransfer(address(_borrowLend), surplusAfterSwap);
            }
            totalDebtRepaid += _borrowLend.repay(totalSurplus);
        }
    }
    }
frontier159 commented 7 months ago

This was intentional, because there may be left over debtToken from a previous rebalance up.

Within a given rebalance up, if the surplus is less than params.repaySurplusThreshold, then it won't do that extra repay.

This means that small surplus amount will be left in the contract until the next rebalance up. It needs to be picked up using balanceOf the next time around.

There is no impact that I can see here from a donation in this form, and no economic gain, flashloan/sandwich isn't possible either since this rebalance call uses flashbots protect and the timing of when it's run is randomised

frontier159 commented 7 months ago

In fact, we explicitly check the resulting A/L is within a min/max too.

https://github.com/TempleDAO/origami-public/blob/185a93e25071b6a110ca190e94a6a826e982b2d6/apps/protocol/contracts/investments/lovToken/managers/OrigamiLovTokenFlashAndBorrowManager.sol#L385

Given that, and that this is expected behaviour for carrying over amounts, this issue can be marked invalid