Fix denial of service borrowing vault #376
Conversation
pedrovalido
changed the base branch from
main
to
protocol/fix/macro-findings-critical-high
|
The issue: Chosen option to address this issue ps.: There's still the lag between someone repaying the full vaults debt and the function being called. In this timeframe, the vault will not function properly until the correctDebt function is called. |
| * | ||
| * @param amount to be borrowed | ||
| */ | ||
| function correctDebt(uint256 amount) external onlyTimelock { |
There was a problem hiding this comment.
@pedrovalido, if I follow the logic of this function correctDebt() the borrowed amount stays stuck in the vault?
There was a problem hiding this comment.
You're correct. I'll fix this
There was a problem hiding this comment.
This is now fixed. Treasury is passed as an argument in the correctDebt function so funds are sent to it
| assertEq(vault.balanceOfDebt(ALICE), 0); | ||
|
|
||
| uint256 maxAmount = vault.maxWithdraw(ALICE); | ||
| do_withdraw(maxAmount, vault, ALICE); |
There was a problem hiding this comment.
Going through the test I realized that in fact if some "troublemaker" paybacks the debt of the vault, users are now free to withdraw their assets without burning their debtShares. I think we need to discuss what behavior we expect.
There was a problem hiding this comment.
Yes, the time in between us calling the troublemaker paying the debt and we calling correctDebt we'll be a problem. I'll come up with some possible solutions so we can discuss them.
There was a problem hiding this comment.
As discussed, I paused the withdraw in the withdraw function and it is not very efficient but couldnt find a better way. Issue was added to the optimizations card in the board.
Also, left a //TODO in the correctDebt function to decide if we should unpause there or not. I remember we touched this issue but didn't reach a conclusion. My suggestion: we can unpause inside the correctDebt function as it is only called by timelock so I think it is safe. Let me know if I'm missing something
There was a problem hiding this comment.
Hey @pedrovalido, as discussed lets proceed to remove the amount argument from the 'correctDebt()` function such that by default we re-establish 1 debtShare == 1 unit of debt asset.
We will also exclude that un-pause from the correct debt function, so that this is a seperate action and the "activation of the vault in more controlled steps. In addition the OZ timelock controller contract allows to execute batch actions too. So in case of desiring an "atomic" un-pausing of the withdraw action once debt is corrected it can be done through the timelock.
Changes have been addressed :) |
|
|
||
| if (totalDebt_ == 0 && supply > 0 && supply > totalDebt_) { | ||
| _pause(VaultActions.Withdraw); | ||
| revert BorrowingVault__withdraw_debtNeedsCorrection(); |
There was a problem hiding this comment.
can you please explain what you intend to do here?
I see a pause on top and a revert added just below
There was a problem hiding this comment.
This is the situation in where some external actor, pays entirely the debt of the vault. By that previously, they will effectively, disable the vault from borrowing operations, if I recall.
Now, if... someone, does pay the vault's debt ever, we created a function to restore debt back, and essentially later decide what to do with the "benefactor" proceeds.
The conditions there, is to avoid withdraws at the moment right after the debt has been paid back.
If you read the conditions: if total debt is zero, meaning... someone paidback the whole debt, and there is debtSharesSupply, meaning.... there were user/s owing funds. Then pause all withdrawals until all gets sort out.
In the case of someone paying off all the debt, we don't want to create "bad debt" position, by allowing users to withdraw the shares and leaving "unbacked" debtShares, once debt is restored.
There was a problem hiding this comment.
Though the revert will do effectively the job. I see now, that state for pause will not persist.
There was a problem hiding this comment.
yep general intention makes sense
Though the revert will do effectively the job. I see now, that state for pause will not persist.
I was more concerned with the above, yep it won't persist
This PR addresses H-1 in Macro audit report.