SOV-5206: introduce loan id guard implementation

contracts/core/State.sol

@@ -216,6 +216,8 @@ contract State is Objects, ReentrancyGuard, SharedReentrancyGuard, Ownable {
 
     address internal pauser;
 
+    mapping(bytes32 => uint256) internal loanIdToBlockNumber;
+
     /**
      * @notice Add signature and target to storage.
      * @dev Protocol is a proxy and requires a way to add every

contracts/modules/LoanClosingsWith.sol

@@ -8,6 +8,7 @@ pragma experimental ABIEncoderV2;
 
 import "../interfaces/ILoanPool.sol";
 import "./LoanClosingsShared.sol";
+import "../reentrancy/LoanIdGuard.sol";
 
 /**
  * @title LoanClosingsWith contract.
@@ -17,7 +18,7 @@ import "./LoanClosingsShared.sol";
  *
  * Loans are liquidated if the position goes below margin maintenance.
  * */
-contract LoanClosingsWith is LoanClosingsShared {
+contract LoanClosingsWith is LoanClosingsShared, LoanIdGuard {
     constructor() public {}
 
     function() external {
@@ -57,6 +58,7 @@ contract LoanClosingsWith is LoanClosingsShared {
         payable
         nonReentrant
         globallyNonReentrant
+        loanIdNonReentrant(loanId)
         iTokenSupplyUnchanged(loanId)
         whenNotPaused
         returns (uint256 loanCloseAmount, uint256 withdrawAmount, address withdrawToken)
@@ -95,6 +97,7 @@ contract LoanClosingsWith is LoanClosingsShared {
         public
         nonReentrant
         globallyNonReentrant
+        loanIdNonReentrant(loanId)
         iTokenSupplyUnchanged(loanId)
         whenNotPaused
         returns (uint256 loanCloseAmount, uint256 withdrawAmount, address withdrawToken)

contracts/modules/LoanOpenings.sol

@@ -13,7 +13,7 @@ import "../mixins/InterestUser.sol";
 import "../swaps/SwapsUser.sol";
 import "../mixins/ModuleCommonFunctionalities.sol";
 import "../connectors/loantoken/lib/MarginTradeStructHelpers.sol";
-
+import "../reentrancy/LoanIdGuard.sol";
 /**
  * @title Loan Openings contract.
  *
@@ -27,7 +27,8 @@ contract LoanOpenings is
     VaultController,
     InterestUser,
     SwapsUser,
-    ModuleCommonFunctionalities
+    ModuleCommonFunctionalities,
+    LoanIdGuard
 {
     constructor() public {}
 
@@ -364,6 +365,9 @@ contract LoanOpenings is
             )
         ];
 
+        // Lock the loan ID to prevent multiple operations in the same block
+        checkAndToggle(loanLocal.id);
+
         // Get required interest.
         uint256 amount = _initializeInterest(
             loanParamsLocal,

contracts/modules/ProtocolSettings.sol

@@ -973,4 +973,15 @@ contract ProtocolSettings is
             defaultPathValue
         );
     }
+
+    /**
+     * @notice Get the block number when a loan ID was last operated on.
+     * This is used to prevent the same loan ID from being operated on multiple times in the same block.
+     *
+     * @param loanId The ID of the loan.
+     * @return The block number (0 if never operated on).
+     */
+    function getLoanIdLastBlockNumber(bytes32 loanId) external view returns (uint256) {
+        return loanIdToBlockNumber[loanId];
+    }
 }

contracts/reentrancy/LoanIdGuard.sol

@@ -0,0 +1,53 @@
+pragma solidity ^0.5.17;
+
+import "../core/State.sol";
+
+/**
+ * @title Abstract contract for loan ID-specific reentrancy guards
+ *
+ * @notice Exposes a modifier `loanIdNonReentrant` that prevents the same loan ID
+ * from being operated on multiple times within the same block.
+ *
+ * @dev This prevents exploits where an attacker:
+ * 1. Opens a loan to manipulate protocol state (e.g., inflate interest rates)
+ * 2. Operates on the same loan again in the same transaction (or same block)
+ * 3. Takes advantage of the manipulated state
+ *
+ * The LoanIdMutex contract address is hardcoded to a deterministically deployed address.
+ * This contract has no state and is safe to add to the inheritance chain of upgradeable contracts.
+ */
+contract LoanIdGuard is State {
+    function checkAndToggle(bytes32 loanId) internal {
+        uint256 lastBlock = loanIdToBlockNumber[loanId];
+
+        // Revert if loan was operated on in the current block
+        require(lastBlock != block.number, "loan ID already used in this block");
+
+        // Mark the loan as operated on in this block
+        loanIdToBlockNumber[loanId] = block.number;
+    }
+
+    /**
+     * @notice This modifier protects functions from being called multiple times on
+     * the same loan ID within the same block.
+     *
+     * @dev Uses block.number tracking in LoanIdMutex. Reverts if the loan was
+     * already operated on in the current block. This prevents flash loan attacks
+     * where a loan is created and closed in the same transaction.
+     *
+     * Note: This also prevents multiple operations on the same loan in different
+     * transactions within the same block, which is an acceptable trade-off for security.
+     *
+     * @param loanId The ID of the loan being operated on.
+     */
+    modifier loanIdNonReentrant(bytes32 loanId) {
+        // Check and mark that this loan is being operated on in this block
+        // Reverts if already operated on in this block
+        checkAndToggle(loanId);
+
+        // Execute the function
+        _;
+
+        // No cleanup needed - block.number naturally differs in the next block
+    }
+}

contracts/testhelpers/FlashBorrowAttack.sol

@@ -0,0 +1,202 @@
+pragma solidity 0.5.17;
+pragma experimental ABIEncoderV2;
+
+import { SafeERC20, IERC20 } from "../openzeppelin/SafeERC20.sol";
+
+/**
+ * @title FlashBorrowAttack
+ * @notice Attack contract that attempts to borrow and close a loan in a single transaction
+ *
+ * This contract demonstrates the vulnerability that LoanIdGuard is designed to prevent:
+ * 1. Borrow a large amount to manipulate interest rates
+ * 2. Immediately close the loan to avoid paying interest
+ * 3. Use the manipulated state to exploit other users
+ *
+ * With LoanIdGuard: The closeWithDeposit call will REVERT because the loan
+ * was created in the same transaction.
+ */
+contract FlashBorrowAttack {
+    using SafeERC20 for IERC20;
+
+    IProtocol public protocol;
+    ILoanToken public loanTokenPool; // The loan pool (iToken)
+    address public underlyingToken; // The underlying ERC20 (e.g., SUSD)
+    address public collateralToken;
+    bytes32 public loanId;
+    uint256 public borrowAmount;
+
+    event AttackAttempted(bytes32 loanId, bool success);
+    event LoanBorrowed(bytes32 loanId, uint256 principal);
+    event LoanClosed(bytes32 loanId);
+
+    constructor(address _protocol, address _loanTokenPool, address _collateralToken) public {
+        protocol = IProtocol(_protocol);
+        loanTokenPool = ILoanToken(_loanTokenPool);
+        underlyingToken = ILoanToken(_loanTokenPool).loanTokenAddress(); // Get underlying token
+        collateralToken = _collateralToken;
+        borrowAmount = 1000 ether;
+    }
+
+    /**
+     * @notice Attempts to execute a flash borrow attack
+     * @dev This function should REVERT with "loan ID already used in this block"
+     *
+     * Attack flow:
+     * 1. Borrow large amount (creates new loan, locks it)
+     * 2. Try to close immediately (should fail due to LoanIdGuard)
+     */
+    function executeAttack(uint256 collateralAmount) external returns (bool) {
+        // Approve collateral for borrowing
+        IERC20(collateralToken).safeApprove(address(loanTokenPool), collateralAmount);
+
+        // Step 1: Borrow through loan token pool (creates and locks the loan ID)
+        (uint256 principal, ) = loanTokenPool.borrow(
+            0, // loanId = 0 means new loan
+            borrowAmount,
+            7884000, // initialLoanDuration ~3 months in seconds
+            collateralAmount,
+            collateralToken,
+            address(this),
+            address(this),
+            ""
+        );
+
+        // Get the created loan ID
+        loanId = _getMyLatestLoanId();
+        emit LoanBorrowed(loanId, principal);
+
+        // Step 2: Try to close immediately in the same transaction
+        // THIS SHOULD REVERT with "loan ID already used in this block"
+        IERC20(underlyingToken).safeApprove(address(protocol), borrowAmount);
+
+        protocol.closeWithDeposit(loanId, address(this), borrowAmount);
+
+        emit LoanClosed(loanId);
+
+        // If we reach here, the attack succeeded (which should NOT happen with the fix)
+        emit AttackAttempted(loanId, true);
+        return true;
+    }
+
+    /**
+     * @notice Borrows without attempting to close (for testing separate transactions)
+     */
+    function justBorrow(uint256 collateralAmount, uint256 _borrowAmount) external {
+        borrowAmount = _borrowAmount;
+
+        IERC20(collateralToken).safeApprove(address(loanTokenPool), collateralAmount);
+
+        (uint256 principal, ) = loanTokenPool.borrow(
+            0,
+            borrowAmount,
+            7884000, // initialLoanDuration ~3 months in seconds
+            collateralAmount,
+            collateralToken,
+            address(this),
+            address(this),
+            ""
+        );
+
+        loanId = _getMyLatestLoanId();
+        emit LoanBorrowed(loanId, principal);
+    }
+
+    /**
+     * @notice Closes an existing loan (for testing separate transactions)
+     */
+    function justClose() external {
+        require(loanId != 0, "No loan to close");
+
+        // Get loan details
+        IProtocol.LoanReturnData memory loan = protocol.getLoan(loanId);
+        uint256 principal = loan.principal;
+
+        IERC20(underlyingToken).safeApprove(address(protocol), principal);
+
+        protocol.closeWithDeposit(loanId, address(this), principal);
+
+        emit LoanClosed(loanId);
+    }
+
+    /**
+     * @notice Gets the most recent loan ID for this contract
+     */
+    function _getMyLatestLoanId() internal view returns (bytes32) {
+        IProtocol.LoanReturnData[] memory loans = protocol.getUserLoans(
+            address(this),
+            0, // start
+            1, // count (we want the latest)
+            0, // loanType (all)
+            false, // isLender
+            false // unsafeOnly
+        );
+
+        require(loans.length > 0, "No loans found");
+        return loans[0].loanId;
+    }
+
+    /**
+     * @notice Fallback to receive tokens
+     */
+    function() external payable {}
+}
+
+/**
+ * @title IProtocol
+ * @notice Minimal interface for Sovryn protocol functions used in the attack
+ */
+interface IProtocol {
+    struct LoanReturnData {
+        bytes32 loanId;
+        address loanToken;
+        address collateralToken;
+        uint256 principal;
+        uint256 collateral;
+        uint256 interestOwedPerDay;
+        uint256 interestDepositRemaining;
+        uint256 startRate;
+        uint256 startMargin;
+        uint256 maintenanceMargin;
+        uint256 currentMargin;
+        uint256 maxLoanTerm;
+        uint256 endTimestamp;
+        uint256 maxLiquidatable;
+        uint256 maxSeizable;
+    }
+
+    function closeWithDeposit(
+        bytes32 loanId,
+        address receiver,
+        uint256 depositAmount
+    ) external returns (uint256 loanCloseAmount, uint256 withdrawAmount, address withdrawToken);
+
+    function getLoan(bytes32 loanId) external view returns (LoanReturnData memory);
+
+    function getUserLoans(
+        address user,
+        uint256 start,
+        uint256 count,
+        uint256 loanType,
+        bool isLender,
+        bool unsafeOnly
+    ) external view returns (LoanReturnData[] memory);
+}
+
+/**
+ * @title ILoanToken
+ * @notice Interface for LoanToken borrow function
+ */
+interface ILoanToken {
+    function loanTokenAddress() external view returns (address);
+
+    function borrow(
+        bytes32 loanId,
+        uint256 withdrawAmount,
+        uint256 initialLoanDuration,
+        uint256 collateralTokenSent,
+        address collateralTokenAddress,
+        address borrower,
+        address receiver,
+        bytes calldata loanDataBytes
+    ) external payable returns (uint256, uint256);
+}