From 6cd951722a83b5e54b8db0881a5465a962204669 Mon Sep 17 00:00:00 2001 From: 0xrusowsky <0xrusowsky@proton.me> Date: Fri, 4 Aug 2023 14:54:16 +0200 Subject: [PATCH] test: ensure callbacks are safe from reentrancy --- .gas-snapshot | 2 + src/test/Cooler.t.sol | 128 ++++++++++++++++++++++++- src/test/mocks/MockMaliciousLender.sol | 24 +++++ 3 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 .gas-snapshot create mode 100644 src/test/mocks/MockMaliciousLender.sol diff --git a/.gas-snapshot b/.gas-snapshot new file mode 100644 index 0000000..b43f8dd --- /dev/null +++ b/.gas-snapshot @@ -0,0 +1,2 @@ +LockTest:test_smartLock() (gas: 7940) +LockTest:test_stdLock() (gas: 7672) \ No newline at end of file diff --git a/src/test/Cooler.t.sol b/src/test/Cooler.t.sol index 1556839..0134642 100644 --- a/src/test/Cooler.t.sol +++ b/src/test/Cooler.t.sol @@ -7,11 +7,11 @@ import {UserFactory} from "test/lib/UserFactory.sol"; import {MockGohm} from "test/mocks/MockGohm.sol"; import {MockERC20} from "solmate/test/utils/mocks/MockERC20.sol"; +import {MockMaliciousLender} from "test/mocks/MockMaliciousLender.sol"; import {Cooler} from "src/Cooler.sol"; import {CoolerFactory} from "src/CoolerFactory.sol"; -// TODO: test repay with callback = false // Tests for Cooler // // [X] constructor @@ -33,8 +33,7 @@ import {CoolerFactory} from "src/CoolerFactory.sol"; // [X] loan is updated // [X] direct (true): new collateral and debt balances are correct // [X] direct (false): new collateral and debt balances are correct -// [ ] callback (true): clearing house is properly called -// [X] callback (false) +// [X] callback (true): cannot perform a reentrancy attack // [X] claimRepaid // [X] loan is updated // [X] lender and cooler new debt balances are correct @@ -47,12 +46,14 @@ import {CoolerFactory} from "src/CoolerFactory.sol"; // [X] loan is updated // [X] request is deactivated // [X] user and cooler new collateral balances are correct +// [X] callback (true): cannot perform a reentrancy attack // [X] provideNewTermsForRoll // [X] only lender can set new terms // [X] request is properly updated // [X] claimDefaulted // [X] only possible after expiry // [X] lender and cooler new collateral balances are correct +// [X] callback (true): cannot perform a reentrancy attack // [X] delegateVoting // [X] only owner can delegate // [X] collateral voting power is properly delegated @@ -486,6 +487,40 @@ contract CoolerTest is Test { assertEq(initLoanCollat - decollatAmount, loanCollat, "outstanding collat"); } } + + function testRevertFuzz_repay_ReentrancyAttack(uint256 amount_, uint256 repayAmount_) public { + // test inputs + repayAmount_ = bound(repayAmount_, 1e10, MAX_DEBT); // min > 0 to have some decollateralization + amount_ = bound(amount_, repayAmount_, MAX_DEBT); + bool directRepay = true; + bool callbackRepay = true; + // test setup + cooler = _initCooler(); + (uint256 reqID, ) = _requestLoan(amount_); + + // Create a malicious lender that reenters on defaults + MockMaliciousLender attacker = new MockMaliciousLender(address(coolerFactory)); + deal(address(debt), address(attacker), amount_); + + vm.startPrank(address(attacker)); + // aprove debt so that it can be transferred from the cooler + debt.approve(address(cooler), amount_); + uint256 loanID = cooler.clearRequest(reqID, directRepay, callbackRepay); + vm.stopPrank(); + + // block.timestamp < loan expiry + vm.warp(block.timestamp + DURATION / 2); + + vm.startPrank(owner); + debt.approve(address(cooler), repayAmount_); + // A reentrancy attack on repayLoan wouldn't provide any economical benefit to the + // attacker, since it would cause the malicious lender to repay the loan on behalf + // of the owner. In this test, the attacker hasn't approved further debt spending + // and therefor the transfer will fail. + vm.expectRevert("TRANSFER_FROM_FAILED"); + cooler.repayLoan(loanID, repayAmount_); + vm.stopPrank(); + } function testRevertFuzz_repayLoan_defaulted(uint256 amount_, uint256 repayAmount_) public { // test inputs @@ -698,6 +733,48 @@ contract CoolerTest is Test { } } + function testRevertFuzz_claimDefaulted_ReentrancyAttack(uint256 amount_) public { + // test inputs + uint256 amount1_ = bound(amount_, 0, MAX_DEBT / 3); + uint256 amount2_ = 2 * amount1_; + bool callbackRepay = false; + bool directRepay = true; + // test setup + cooler = _initCooler(); + // Request ID = 1 + vm.startPrank(owner); + collateral.approve(address(cooler), amount1_); + uint256 reqID1 = cooler.requestLoan(amount1_, INTEREST_RATE, LOAN_TO_COLLATERAL, DURATION); + vm.stopPrank(); + // Request ID = 2 + vm.startPrank(others); + collateral.approve(address(cooler), amount2_); + uint256 reqID2 = cooler.requestLoan(amount2_, INTEREST_RATE, LOAN_TO_COLLATERAL, DURATION); + vm.stopPrank(); + + // Create a malicious lender that reenters on defaults + MockMaliciousLender attacker = new MockMaliciousLender(address(coolerFactory)); + deal(address(debt), address(attacker), amount1_ + amount2_); + + vm.startPrank(address(attacker)); + // aprove debt so that it can be transferred from the cooler + debt.approve(address(cooler), amount1_ + amount2_); + uint256 loanID1 = cooler.clearRequest(reqID1, directRepay, callbackRepay); + uint256 loanID2 = cooler.clearRequest(reqID2, directRepay, callbackRepay); + vm.stopPrank(); + + // block.timestamp > loan expiry + vm.warp(block.timestamp + DURATION + 1); + + vm.prank(lender); + cooler.claimDefaulted(loanID1); + + // A reentrancy attack on claimDefaulted() doesn't have any impact thanks to the usage of the + // Check-Effects-Interaction pattern. Since the loan storage is emptied at the begining, + // a reentrancy attack is useless and ends in a second transfer of 0 collateral tokens to the attacker. + assertEq(cooler.collateralFor(amount2_, LOAN_TO_COLLATERAL), collateral.balanceOf(address(cooler))); + } + function testRevertFuzz_defaulted_notExpired(uint256 amount_) public { // test inputs amount_ = bound(amount_, 0, MAX_DEBT); @@ -926,6 +1003,51 @@ contract CoolerTest is Test { cooler.rollLoan(loanID); } + function testRevertFuzz_roll_ReentrancyAttack(uint256 amount_) public { + // test inputs + amount_ = bound(amount_, 0, MAX_DEBT / 2); + bool directRepay = true; + bool callbackRepay = true; + // test setup + cooler = _initCooler(); + (uint256 reqID, ) = _requestLoan(amount_); + + // Create a malicious lender that reenters on defaults + MockMaliciousLender attacker = new MockMaliciousLender(address(coolerFactory)); + deal(address(debt), address(attacker), amount_); + + vm.startPrank(address(attacker)); + // aprove debt so that it can be transferred from the cooler + debt.approve(address(cooler), amount_); + uint256 loanID = cooler.clearRequest(reqID, directRepay, callbackRepay); + vm.stopPrank(); + + // block.timestamp < loan expiry + vm.warp(block.timestamp + DURATION / 2); + + vm.prank(address(attacker)); + cooler.provideNewTermsForRoll( + loanID, + INTEREST_RATE * 2, + LOAN_TO_COLLATERAL / 2, + DURATION * 2 + ); + + (, uint256 loanAmount,, uint256 loanCollat,,,,) = cooler.loans(loanID); + uint256 rollCollat = loanAmount * DECIMALS / (LOAN_TO_COLLATERAL / 2); + uint256 newCollat = rollCollat > loanCollat ? rollCollat - loanCollat : 0; + + vm.startPrank(owner); + // aprove collateral so that it can be transferred by cooler + collateral.approve(address(cooler), newCollat); + // A reentrancy attack on rollLoan() doesn't have any impact thanks to the usage of the + // Check-Effects-Interaction pattern. Since the loan request is deactivated at the begining, + // a reentrancy attack attemp reverts with a NotRollable() error. + vm.expectRevert(Cooler.NotRollable.selector); + cooler.rollLoan(loanID); + vm.stopPrank(); + } + function testRevertFuzz_roll_defaulted(uint256 amount_) public { // test inputs amount_ = bound(amount_, 0, MAX_DEBT); diff --git a/src/test/mocks/MockMaliciousLender.sol b/src/test/mocks/MockMaliciousLender.sol new file mode 100644 index 0000000..119f501 --- /dev/null +++ b/src/test/mocks/MockMaliciousLender.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: AGPL-3.0 +pragma solidity ^0.8.0; + +import {CoolerCallback} from "src/CoolerCallback.sol"; +import {Cooler} from "src/Cooler.sol"; + +contract MockMaliciousLender is CoolerCallback { + constructor(address coolerFactory_) CoolerCallback(coolerFactory_) {} + + /// @notice Callback function that handles repayments. Override for custom logic. + function _onRepay(uint256 loanID_, uint256 amount_) internal override { + Cooler(msg.sender).repayLoan(loanID_, amount_); + } + + /// @notice Callback function that handles rollovers. + function _onRoll(uint256 loanID_, uint256 newDebt, uint256 newCollateral) internal override { + Cooler(msg.sender).rollLoan(loanID_); + } + + /// @notice Callback function that handles defaults. + function _onDefault(uint256 loanID_, uint256 debt, uint256 collateral) internal override { + Cooler(msg.sender).claimDefaulted(loanID_); + } +} \ No newline at end of file