test: reentrancy attacks (#35)

.gas-snapshot

@@ -0,0 +1,2 @@
+LockTest:test_smartLock() (gas: 7940)
+LockTest:test_stdLock() (gas: 7672)

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);

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_);
+    }
+}