SOV-5206: introduce loan id guard implementation

contracts/core/State.sol

@@ -13,6 +13,7 @@ import "../openzeppelin/Ownable.sol";
 import "../openzeppelin/SafeMath.sol";
 import "../interfaces/IWrbtcERC20.sol";
 import "../reentrancy/SharedReentrancyGuard.sol";
+import "../reentrancy/LoanIdGuard.sol";
 
 /**
  * @title State contract.
@@ -21,7 +22,7 @@ import "../reentrancy/SharedReentrancyGuard.sol";
  *
  * This contract contains the storage values of the Protocol.
  * */
-contract State is Objects, ReentrancyGuard, SharedReentrancyGuard, Ownable {
+contract State is Objects, ReentrancyGuard, SharedReentrancyGuard, LoanIdGuard, Ownable {
     using SafeMath for uint256;
     using EnumerableAddressSet for EnumerableAddressSet.AddressSet; // enumerable map of addresses
     using EnumerableBytes32Set for EnumerableBytes32Set.Bytes32Set; // enumerable map of bytes32 or addresses

contracts/modules/LoanClosingsWith.sol

@@ -57,6 +57,7 @@ contract LoanClosingsWith is LoanClosingsShared {
         payable
         nonReentrant
         globallyNonReentrant
+        loanIdNonReentrant(loanId)
         iTokenSupplyUnchanged(loanId)
         whenNotPaused
         returns (uint256 loanCloseAmount, uint256 withdrawAmount, address withdrawToken)
@@ -95,6 +96,7 @@ contract LoanClosingsWith is LoanClosingsShared {
         public
         nonReentrant
         globallyNonReentrant
+        loanIdNonReentrant(loanId)
         iTokenSupplyUnchanged(loanId)
         whenNotPaused
         returns (uint256 loanCloseAmount, uint256 withdrawAmount, address withdrawToken)

contracts/modules/LoanOpenings.sol

@@ -364,6 +364,9 @@ contract LoanOpenings is
             )
         ];
 
+        // Lock the loan ID to prevent multiple operations in the same block
+        LOAN_ID_MUTEX.checkAndToggle(loanLocal.id);
+
         // Get required interest.
         uint256 amount = _initializeInterest(
             loanParamsLocal,

contracts/reentrancy/LoanIdGuard.sol

@@ -0,0 +1,56 @@
+pragma solidity ^0.5.17;
+
+import "./LoanIdMutex.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 {
+    /**
+     * @notice The address of the LoanIdMutex contract.
+     *
+     * @dev Hardcoded to avoid changing the memory layout of derived contracts.
+     * The LoanIdMutex is deployed to the same address on all networks using
+     * deterministic deployment (similar to ERC1820Registry).
+     *
+     * @dev Internal visibility allows derived contracts to access it directly when needed
+     * (e.g., in LoanOpenings.sol where the loanId is created dynamically).
+     */
+    LoanIdMutex internal constant LOAN_ID_MUTEX =
+        LoanIdMutex(0x6B8F44710CdCC7D5A5F60a3665F7B181Cda7ED27);
+
+    /**
+     * @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
+        LOAN_ID_MUTEX.checkAndToggle(loanId);
+
+        // Execute the function
+        _;
+
+        // No cleanup needed - block.number naturally differs in the next block
+    }
+}

contracts/reentrancy/LoanIdMutex.sol

@@ -0,0 +1,53 @@
+pragma solidity ^0.5.17;
+
+/**
+ * @title Loan ID Mutex contract
+ *
+ * @notice A mutex contract that prevents multiple operations on the same loan ID
+ * within the same block. This prevents exploits where an attacker manipulates
+ * protocol state (like interest rates) and then operates on the same loan in the
+ * same transaction to take advantage of the temporary state change.
+ *
+ * @dev Uses block.number to track when each loan ID was last operated on.
+ * This blocks any operations on the same loan ID within the same block, whether
+ * in the same transaction or different transactions. This is an acceptable trade-off
+ * to prevent flash loan attacks.
+ */
+contract LoanIdMutex {
+    /**
+     * @notice Mapping from loan ID to the block number where it was last operated on.
+     *
+     * @dev 0 = never operated on
+     *      non-zero = last operated in that block number
+     */
+    mapping(bytes32 => uint256) public loanIdToBlockNumber;
+
+    /**
+     * @notice Check and mark that a loan ID is being operated on in this block.
+     *
+     * @dev Reverts if the loan was already operated on in the current block.
+     * This prevents both sequential operations in the same transaction AND
+     * multiple transactions on the same loan in the same block.
+     *
+     * @param loanId The ID of the loan to check and mark.
+     */
+    function checkAndToggle(bytes32 loanId) external {
+        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 Get the block number when a loan ID was last operated on.
+     *
+     * @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/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);
+}

contracts/testhelpers/LoanIdMutexTester.sol

@@ -0,0 +1,25 @@
+pragma solidity ^0.5.17;
+
+import "../reentrancy/LoanIdMutex.sol";
+
+/**
+ * @title Helper contract to test LoanIdMutex same-block protection
+ * @notice This contract calls checkAndToggle twice in a single transaction
+ * to verify that the mutex properly blocks same-block operations
+ */
+contract LoanIdMutexTester {
+    LoanIdMutex public loanIdMutex;
+
+    constructor(address _loanIdMutex) public {
+        loanIdMutex = LoanIdMutex(_loanIdMutex);
+    }
+
+    /**
+     * @notice Attempts to call checkAndToggle twice on the same loan ID
+     * @dev This should revert on the second call since both are in the same transaction/block
+     */
+    function doubleCheckAndToggle(bytes32 loanId) external {
+        loanIdMutex.checkAndToggle(loanId);
+        loanIdMutex.checkAndToggle(loanId); // This should revert
+    }
+}