Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix denial of service borrowing vault #376

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0b786e8
test(protocol): denial of service borrowing vault
pedrovalido Mar 15, 2023
f1ae94a
fix(protocol): incorrect contract name
pedrovalido Mar 15, 2023
99e3f46
test(protocol): denial of service borrowing vault more use cases
pedrovalido Mar 15, 2023
ef730bf
fix(protocol): ops not counting with previous balances
pedrovalido Mar 20, 2023
c2aed27
fix(protocol): add correctDebt to counter dos
pedrovalido Mar 28, 2023
20c5bb0
fix(protocol): fix tests, test with correctDebt function
pedrovalido Mar 28, 2023
ecfacbe
fix(protocol): add pre condition of shares not zero
pedrovalido Mar 28, 2023
0ad0438
fix(protocol): use correctedAmount variable, also check balance of vault
pedrovalido Mar 29, 2023
fa087ea
fix(protocol): borrowed money stuck in vault
pedrovalido Apr 13, 2023
0fba46a
fix(protocol): check total debt before withdraw
pedrovalido Apr 13, 2023
a92048c
fix(protocol): add before withdraw hook
pedrovalido Apr 13, 2023
33c62dc
test(protocol): check withdraw is paused until correctDebt is called
pedrovalido Apr 13, 2023
a087364
fix(protocol): unpause only when paused
pedrovalido Apr 14, 2023
75c4deb
fix(protocol): correct scope of tests
pedrovalido Apr 14, 2023
67c50be
fix(protocol): remove unused comment
pedrovalido Apr 14, 2023
49a5f2e
fix(protocol): remove amount and unpause from correctDebt
pedrovalido Apr 19, 2023
bb24fc6
fix(protocol): remove amount and unpause from correctDebt
pedrovalido Apr 19, 2023
0fe9d2a
chore(protocol): remove compiling warnings and tiny optimizations
0xdcota Apr 20, 2023
bfc6e66
Merge branch 'protocol/fix/macro-findings-critical-high' into protoco…
0xdcota Apr 20, 2023
50cf119
chore(protocol): fmt after resolving merge conflicts
0xdcota Apr 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/protocol/src/abstracts/BaseVault.sol
Expand Up @@ -589,6 +589,7 @@ abstract contract BaseVault is ERC20, SystemAccessControl, PausableVault, VaultP
uint256 shares
)
internal
virtual
whenNotPaused(VaultActions.Withdraw)
{
_burn(owner, shares);
Expand Down
2 changes: 1 addition & 1 deletion packages/protocol/src/routers/ConnextRouter.sol
Expand Up @@ -315,7 +315,7 @@ contract ConnextRouter is BaseRouter, IXReceiver {

emit XCalled(
transferId, msg.sender, routerByDomain[destDomain], destDomain, asset, amount, callData
);
);

return beneficiary_;
}
Expand Down
53 changes: 53 additions & 0 deletions packages/protocol/src/vaults/borrowing/BorrowingVault.sol
Expand Up @@ -67,6 +67,8 @@ contract BorrowingVault is BaseVault {
error BorrowingVault__borrow_slippageTooHigh();
error BorrowingVault__payback_slippageTooHigh();
error BorrowingVault__burnDebtShares_amountExceedsBalance();
error BorrowingVault__correctDebt_noNeedForCorrection();
error BorrowingVault__withdraw_debtNeedsCorrection();

/*///////////////////
Liquidation controls
Expand Down Expand Up @@ -301,6 +303,37 @@ contract BorrowingVault is BaseVault {
return shares;
}

/**
* @dev Checks if vault debt has been paid off externally and pauses the vault if so.
* Will call withdraw if normal conditions are met.
*
* @param caller or {msg.sender}
* @param receiver to whom `assets` amount will be transferred to
* @param owner to whom `shares` will be burned
* @param assets amount transferred during this withraw
* @param shares amount burned to `owner` during this withdraw
*/
function _withdraw(
address caller,
address receiver,
address owner,
uint256 assets,
uint256 shares
)
internal
override
whenNotPaused(VaultActions.Withdraw)
{
uint256 totalDebt_ = totalDebt();
uint256 supply = debtSharesSupply;

if (totalDebt_ == 0 && supply > 0 && supply > totalDebt_) {
_pause(VaultActions.Withdraw);
revert BorrowingVault__withdraw_debtNeedsCorrection();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please explain what you intend to do here?
I see a pause on top and a revert added just below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though the revert will do effectively the job. I see now, that state for pause will not persist.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to #536

}
super._withdraw(caller, receiver, owner, assets, shares);
}

/*///////////////////////
Borrow allowances
///////////////////////*/
Expand Down Expand Up @@ -738,4 +771,24 @@ contract BorrowingVault is BaseVault {

emit ProvidersChanged(providers);
}

/**
* @notice In case someone repays this vault's debt directly to the lending provider,
* a borrow is done with the amount that was repaid so no issues regarding
* the relationship between debt and debt shares occurs.
*
* @param treasury address to receive the borrowed amount
*/
function correctDebt(address treasury) external onlyTimelock {
uint256 vaultDebt = totalDebt();
uint256 vaultDebtShares = debtSharesSupply;

// Check if there is need for correction.
if (vaultDebt >= vaultDebtShares || vaultDebt != 0 || vaultDebtShares == 0) {
revert BorrowingVault__correctDebt_noNeedForCorrection();
}

_executeProviderAction(vaultDebtShares, "borrow", activeProvider);
SafeERC20.safeTransfer(IERC20(debtAsset()), treasury, vaultDebtShares);
}
}
Expand Up @@ -117,7 +117,7 @@ contract BorrowingVaultFactory is VaultDeployer {

emit DeployBorrowingVault(
vault, vdata.asset, vdata.debtAsset, vdata.name, vdata.symbol, vdata.salt
);
);
}

/**
Expand Down
98 changes: 98 additions & 0 deletions packages/protocol/test/forking/mainnet/DenialOfService.t.sol
@@ -0,0 +1,98 @@
// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.15;

import "forge-std/console.sol";
import {Routines} from "../../utils/Routines.sol";
import {ForkingSetup} from "../ForkingSetup.sol";
import {AaveV2} from "../../../src/providers/mainnet/AaveV2.sol";
import {ILendingProvider} from "../../../src/interfaces/ILendingProvider.sol";
import {BorrowingVault} from "../../../src/vaults/borrowing/BorrowingVault.sol";
import {IERC20} from "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";
import {IV2Pool} from "../../../src/interfaces/aaveV2/IV2Pool.sol";
import {SafeERC20} from "openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
import {PausableVault} from "../../../src/abstracts/PausableVault.sol";

contract DenialOfServiceTest is Routines, ForkingSetup {
using SafeERC20 for IERC20;

uint256 public constant TREASURY_PK = 0xF;
address public TREASURY = vm.addr(TREASURY_PK);

ILendingProvider public aaveV2;

uint256 public TROUBLEMAKER_PK = 0x1122;
address public TROUBLEMAKER = vm.addr(TROUBLEMAKER_PK);

uint256 public constant DEPOSIT_AMOUNT = 1 ether;
uint256 public constant BORROW_AMOUNT = 800 * 1e6;

IV2Pool public aaveV2pool;

function setUp() public {
setUpFork(MAINNET_DOMAIN);

vm.label(TROUBLEMAKER, "TROUBLEMAKER");

aaveV2 = new AaveV2();
ILendingProvider[] memory providers = new ILendingProvider[](1);
providers[0] = aaveV2;

deploy(providers);

aaveV2pool = IV2Pool(0x7d2768dE32b0b80b7a3454c06BdAc94A69DDc7A9);
}

function testFail_tryWithdrawWithoutCorrectDebt() public {
// Alice deposits and borrows
do_deposit(DEPOSIT_AMOUNT, vault, ALICE);
do_borrow(BORROW_AMOUNT, vault, ALICE);

// Troublemaker pays vaults debt
deal(debtAsset, TROUBLEMAKER, BORROW_AMOUNT * 10);
vm.startPrank(TROUBLEMAKER);
IERC20(debtAsset).safeApprove(address(aaveV2pool), BORROW_AMOUNT);
aaveV2pool.repay(vault.debtAsset(), BORROW_AMOUNT, 2, address(vault));
vm.stopPrank();

assertEq(vault.balanceOf(ALICE), DEPOSIT_AMOUNT);
assertEq(vault.balanceOfDebt(ALICE), 0);

//Withdraw will fail until debt is corrected and withdraw is unpaused
uint256 maxAmount = vault.maxWithdraw(ALICE);
do_withdraw(maxAmount, vault, ALICE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

function test_correctDebtUnpauseAndWithdraw() public {
uint256 amountCorrected = BORROW_AMOUNT;

do_deposit(DEPOSIT_AMOUNT, vault, ALICE);
do_borrow(BORROW_AMOUNT, vault, ALICE);

// Troublemaker pays vaults debt
deal(debtAsset, TROUBLEMAKER, BORROW_AMOUNT * 10);
vm.startPrank(TROUBLEMAKER);
//TODO check this after rounding issue is fixed
//approve more than needed because of rounding issues
IERC20(debtAsset).safeApprove(address(aaveV2pool), BORROW_AMOUNT + 10);
//pay full amount, sometimes bigger than BORROW_AMOUNT
aaveV2pool.repay(
vault.debtAsset(), aaveV2.getBorrowBalance(address(vault), vault), 2, address(vault)
);
vm.stopPrank();

bytes memory data = abi.encodeWithSelector(BorrowingVault.correctDebt.selector, TREASURY);
_callWithTimelock(address(vault), data);
skip(2 days);

//try to payback and withdraw and check it works
do_payback(BORROW_AMOUNT, vault, ALICE);
do_withdraw(DEPOSIT_AMOUNT, vault, ALICE);

//DEPOSIT_AMOUNT has built up some interest after the call with timelock (lets assume 1%)
assertApproxEqAbs(vault.balanceOf(ALICE), 0, DEPOSIT_AMOUNT / 100);

//check the actual corrected amount is in TREASURY
assertEq(aaveV2.getBorrowBalance(address(vault), vault), 0);
assertEq(IERC20(debtAsset).balanceOf(TREASURY), amountCorrected);
}
}
11 changes: 8 additions & 3 deletions packages/protocol/test/utils/Routines.sol
Expand Up @@ -20,6 +20,9 @@ contract Routines is Test {
}

function do_deposit(uint256 amount, IVault v, address from) internal {
uint256 mintedShares = v.balanceOf(from);
uint256 assetBalanceBefore = v.convertToAssets(mintedShares);

address asset = v.asset();
deal(asset, from, amount);

Expand All @@ -31,10 +34,10 @@ contract Routines is Test {
vm.warp(block.timestamp + 13 seconds);
vm.roll(block.number + 1);

uint256 mintedShares = v.balanceOf(from);
mintedShares = v.balanceOf(from);
uint256 assetBalance = v.convertToAssets(mintedShares);

assertApproxEqAbs(assetBalance, amount, amount / 1000);
assertApproxEqAbs(assetBalance - assetBalanceBefore, amount, amount / 1000);

vm.warp(block.timestamp - 13 seconds);
vm.roll(block.number - 1);
Expand All @@ -51,10 +54,12 @@ contract Routines is Test {
}

function do_borrow(uint256 amount, IVault v, address from) internal {
uint256 prevDebt = IERC20(v.debtAsset()).balanceOf(from);

vm.prank(from);
v.borrow(amount, from, from);

assertEq(IERC20(v.debtAsset()).balanceOf(from), amount);
assertEq(IERC20(v.debtAsset()).balanceOf(from) - prevDebt, amount);
}

function do_payback(uint256 amount, IVault v, address from) internal {
Expand Down