Fix/audit fixes round 7 (#93)

* fix: g-4 BNPLHelper.activeUserDataHash can be deleted, also rearranged BNPL helper

* fix: g-7 and g-8

* fix: G-5. Unchecking arithmetics operations that can’t underflow/overflow

* fix: g-1 Consider checking the allowance before calling ERC20.approve()

.gas-snapshot

@@ -4,7 +4,7 @@ IntegrationTestCaveats:testOriginateUnapprovedFulfiller() (gas: 332147)
 IntegrationTestCaveats:testOriginateWBorrowerApproval() (gas: 283288)
 IntegrationTestCaveats:testOriginateWCaveatsAsBorrower() (gas: 306526)
 IntegrationTestCaveats:testOriginateWCaveatsExpired() (gas: 159299)
-IntegrationTestCaveats:testOriginateWCaveatsIncrementedNonce() (gas: 168077)
+IntegrationTestCaveats:testOriginateWCaveatsIncrementedNonce() (gas: 167791)
 IntegrationTestCaveats:testOriginateWCaveatsInvalidSalt() (gas: 284586)
 IntegrationTestCaveats:testOriginateWCaveatsInvalidSaltManual() (gas: 141962)
 IntegrationTestCaveats:testOriginateWLenderApproval() (gas: 283408)
@@ -30,20 +30,20 @@ TestCustodian:testCannotLazyMintTwice() (gas: 82131)
 TestCustodian:testCannotMintInvalidLoanInvalidCustodian() (gas: 72437)
 TestCustodian:testCannotMintInvalidLoanValidCustodian() (gas: 77947)
 TestCustodian:testCustodianCannotBeAuthorized() (gas: 142196)
-TestCustodian:testCustodySelector() (gas: 2706027)
+TestCustodian:testCustodySelector() (gas: 2731904)
 TestCustodian:testDefaultCustodySelectorRevert() (gas: 72420)
-TestCustodian:testGenerateOrderInvalidPostRepayment() (gas: 173141)
-TestCustodian:testGenerateOrderInvalidPostSettlement() (gas: 163282)
-TestCustodian:testGenerateOrderRepay() (gas: 177299)
-TestCustodian:testGenerateOrderRepayAsRepayApprovedBorrower() (gas: 193763)
-TestCustodian:testGenerateOrderRepayERC1155AndERC20() (gas: 875091)
-TestCustodian:testGenerateOrderRepayERC1155AndERC20HandlerAuthorized() (gas: 804777)
+TestCustodian:testGenerateOrderInvalidPostRepayment() (gas: 173073)
+TestCustodian:testGenerateOrderInvalidPostSettlement() (gas: 163214)
+TestCustodian:testGenerateOrderRepay() (gas: 177231)
+TestCustodian:testGenerateOrderRepayAsRepayApprovedBorrower() (gas: 193695)
+TestCustodian:testGenerateOrderRepayERC1155AndERC20() (gas: 876053)
+TestCustodian:testGenerateOrderRepayERC1155AndERC20HandlerAuthorized() (gas: 804641)
 TestCustodian:testGenerateOrderRepayInvalidHookAddress() (gas: 97653)
 TestCustodian:testGenerateOrderRepayInvalidHookReturnType() (gas: 92014)
 TestCustodian:testGenerateOrderRepayNotBorrower() (gas: 106914)
-TestCustodian:testGenerateOrderSettlement() (gas: 155015)
-TestCustodian:testGenerateOrderSettlementHandlerAuthorized() (gas: 160412)
-TestCustodian:testGenerateOrderSettlementNoActiveLoan() (gas: 163460)
+TestCustodian:testGenerateOrderSettlement() (gas: 154947)
+TestCustodian:testGenerateOrderSettlementHandlerAuthorized() (gas: 160344)
+TestCustodian:testGenerateOrderSettlementNoActiveLoan() (gas: 163392)
 TestCustodian:testGenerateOrderSettlementUnauthorized() (gas: 101879)
 TestCustodian:testGenerateOrdersWithLoanStartAtBlockTimestampInvalidLoan() (gas: 461629)
 TestCustodian:testGetBorrower() (gas: 78621)
@@ -58,11 +58,11 @@ TestCustodian:testName() (gas: 7099)
 TestCustodian:testNonPayableFunctions() (gas: 215173)
 TestCustodian:testOnlySeaport() (gas: 17918)
 TestCustodian:testPreviewOrderNoActiveLoan() (gas: 105759)
-TestCustodian:testPreviewOrderRepay() (gas: 230309)
-TestCustodian:testPreviewOrderSettlement() (gas: 192037)
+TestCustodian:testPreviewOrderRepay() (gas: 230241)
+TestCustodian:testPreviewOrderSettlement() (gas: 191969)
 TestCustodian:testPreviewOrderSettlementInvalidFufliller() (gas: 108320)
 TestCustodian:testPreviewOrderSettlementInvalidRepayer() (gas: 117031)
-TestCustodian:testRatifyOrder() (gas: 184120)
+TestCustodian:testRatifyOrder() (gas: 184052)
 TestCustodian:testSeaportMetadata() (gas: 8644)
 TestCustodian:testSupportsInterface() (gas: 9428)
 TestCustodian:testSymbol() (gas: 7216)
@@ -73,30 +73,28 @@ TestLenderEnforcer:testLERevertInvalidLoanTerms() (gas: 81096)
 TestLenderEnforcer:testLEValidLoanTerms() (gas: 72169)
 TestLenderEnforcer:testLEValidLoanTermsAnyBorrower() (gas: 72323)
 TestLenderEnforcer:testLEValidLoanTermsWithAdditionalTransfers() (gas: 73525)
-TestLoanCombinations:testLoan20For721SimpleInterestDutchFixedRepay() (gas: 591976)
-TestLoanCombinations:testLoan20for20SimpleInterestDutchFixedRepay() (gas: 599167)
-TestLoanCombinations:testLoan721for20SimpleInterestDutchFixedRepay() (gas: 590438)
-TestLoanCombinations:testLoanAstariaSettlementRepay() (gas: 580300)
-TestNewLoan:testBuyNowPayLater() (gas: 2873894)
-TestNewLoan:testInvalidSenderBNPL() (gas: 1613098)
-TestNewLoan:testInvalidUserDataHashBNPL() (gas: 1615699)
-TestNewLoan:testNewLoanAs1271ProxyAccountSender() (gas: 874115)
-TestNewLoan:testNewLoanAs1271ProxyAccountThirdPartyFiller() (gas: 885129)
+TestLoanCombinations:testLoan20For721SimpleInterestDutchFixedRepay() (gas: 593006)
+TestLoanCombinations:testLoan20for20SimpleInterestDutchFixedRepay() (gas: 600197)
+TestLoanCombinations:testLoan721for20SimpleInterestDutchFixedRepay() (gas: 590370)
+TestLoanCombinations:testLoanAstariaSettlementRepay() (gas: 580232)
+TestNewLoan:testBuyNowPayLater() (gas: 3018439)
+TestNewLoan:testNewLoanAs1271ProxyAccountSender() (gas: 874205)
+TestNewLoan:testNewLoanAs1271ProxyAccountThirdPartyFiller() (gas: 885161)
 TestNewLoan:testNewLoanERC721CollateralDefaultTerms2() (gas: 429545)
 TestNewLoan:testNewLoanRefinance() (gas: 590031)
-TestNewLoan:testNewLoanViaOriginatorBorrowerApprovalAndLenderApproval() (gas: 326073)
+TestNewLoan:testNewLoanViaOriginatorBorrowerApprovalAndLenderApproval() (gas: 325963)
 TestNewLoan:testNewLoanViaOriginatorLenderApproval() (gas: 384859)
-TestNewLoan:testSettleLoan() (gas: 642171)
+TestNewLoan:testSettleLoan() (gas: 642103)
 TestPausableNonReentrant:testNotOwner() (gas: 21254)
 TestPausableNonReentrant:testPauseAndUnpause() (gas: 22621)
 TestPausableNonReentrant:testReentrancy() (gas: 15404)
 TestPausableNonReentrant:testUnpauseWhenNotPaused() (gas: 12582)
-TestRepayLoan:testRepayLoanApprovedRepayer() (gas: 667169)
-TestRepayLoan:testRepayLoanBase() (gas: 599955)
+TestRepayLoan:testRepayLoanApprovedRepayer() (gas: 667101)
+TestRepayLoan:testRepayLoanBase() (gas: 599887)
 TestRepayLoan:testRepayLoanGenerateOrderNotSeaport() (gas: 438687)
-TestRepayLoan:testRepayLoanInSettlement() (gas: 585892)
+TestRepayLoan:testRepayLoanInSettlement() (gas: 585679)
 TestRepayLoan:testRepayLoanInvalidRepayer() (gas: 603988)
-TestRepayLoan:testRepayLoanThatDoesNotExist() (gas: 858823)
+TestRepayLoan:testRepayLoanThatDoesNotExist() (gas: 858619)
 TestSimpleInterestPricing:test_calculateInterest() (gas: 881296)
 TestSimpleInterestPricing:test_getPaymentConsideration() (gas: 928510)
 TestSimpleInterestPricing:test_getRefinanceConsideration() (gas: 919314)
@@ -116,10 +114,10 @@ TestStarport:testDefaultFeeRake1() (gas: 387812)
 TestStarport:testDefaultFeeRake2() (gas: 450155)
 TestStarport:testDefaultFeeRakeExoticDebt() (gas: 397641)
 TestStarport:testEIP712Signing() (gas: 83089)
-TestStarport:testExoticDebtWithCustomPricingAndRepayment() (gas: 1237839)
-TestStarport:testExoticDebtWithCustomPricingAndSettlement() (gas: 1692860)
+TestStarport:testExoticDebtWithCustomPricingAndRepayment() (gas: 1237771)
+TestStarport:testExoticDebtWithCustomPricingAndSettlement() (gas: 1692792)
 TestStarport:testExoticDebtWithNoCaveatsNotAsBorrower() (gas: 376819)
-TestStarport:testIncrementCaveatNonce() (gas: 35292)
+TestStarport:testIncrementCaveatNonce() (gas: 35006)
 TestStarport:testInitializedFlagSetProperly() (gas: 67438)
 TestStarport:testInvalidAdditionalTransfersOriginate() (gas: 230392)
 TestStarport:testInvalidAdditionalTransfersRefinance() (gas: 170796)
@@ -140,8 +138,8 @@ TestStarport:testTokenNoCodeCollateral() (gas: 150695)
 TestStarport:testTokenNoCodeDebt() (gas: 180946)
 TestStarport:testUnpause() (gas: 17363)
 TestStrategistOriginator:testEncodeWithAccountCounter() (gas: 12330)
-TestStrategistOriginator:testGetStrategistData() (gas: 1809233)
-TestStrategistOriginator:testIncrementCounterAsStrategist() (gas: 38767)
+TestStrategistOriginator:testGetStrategistData() (gas: 1790990)
+TestStrategistOriginator:testIncrementCounterAsStrategist() (gas: 38465)
 TestStrategistOriginator:testIncrementCounterNotAuthorized() (gas: 13423)
 TestStrategistOriginator:testInvalidCollateral() (gas: 211094)
 TestStrategistOriginator:testInvalidDeadline() (gas: 216915)

package.json

@@ -13,7 +13,8 @@
   },
   "scripts": {
     "prepare": "husky install",
-    "snapshot": "forge snapshot --ffi --no-match-path '*fuzz*'"
+    "snapshot": "forge snapshot --ffi --no-match-path '*fuzz*'",
+    "test": "forge test --ffi --no-match-path *fuzz*"
   },
   "devDependencies": {
     "husky": "^8.0.0",

src/BNPLHelper.sol

@@ -30,7 +30,7 @@ pragma solidity ^0.8.17;
 import {Starport} from "./Starport.sol";
 import {CaveatEnforcer} from "./enforcers/CaveatEnforcer.sol";
 import {AdditionalTransfer} from "./lib/StarportLib.sol";
-
+import {Ownable} from "solady/src/auth/Ownable.sol";
 import {Seaport} from "seaport/contracts/Seaport.sol";
 import {
     AdvancedOrder,
@@ -69,7 +69,7 @@ interface ERC20 {
     function transfer(address, uint256) external returns (bool);
 }
 
-contract BNPLHelper is IFlashLoanRecipient {
+contract BNPLHelper is IFlashLoanRecipient, Ownable {
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
     /*                       CUSTOM ERRORS                        */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
@@ -81,7 +81,7 @@ contract BNPLHelper is IFlashLoanRecipient {
     /*                  CONSTANTS AND IMMUTABLES                  */
     /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/
 
-    address private constant VAULT = address(0xBA12222222228d8Ba445958a75a0704d566BF2C8);
+    address private VAULT;
 
     /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
     /*                          STORAGE                           */
@@ -105,9 +105,12 @@ contract BNPLHelper is IFlashLoanRecipient {
         Fulfillment[] fulfillments;
     }
 
-    function makeFlashLoan(address[] calldata tokens, uint256[] calldata amounts, bytes calldata userData) external {
-        activeUserDataHash = keccak256(userData);
+    constructor(address vault, address owner) {
+        VAULT = vault;
+        _initializeOwner(owner);
+    }
 
+    function makeFlashLoan(address[] calldata tokens, uint256[] calldata amounts, bytes calldata userData) external {
         IVault(VAULT).flashLoan(this, tokens, amounts, userData);
     }
 
@@ -117,15 +120,6 @@ contract BNPLHelper is IFlashLoanRecipient {
         uint256[] calldata feeAmounts,
         bytes calldata userData
     ) external override {
-        if (msg.sender != VAULT) {
-            revert SenderNotVault();
-        }
-
-        if (activeUserDataHash != keccak256(userData)) {
-            revert InvalidUserDataProvided();
-        }
-        delete activeUserDataHash;
-
         Execution memory execution = abi.decode(userData, (Execution));
 
         // Approve seaport
@@ -157,4 +151,8 @@ contract BNPLHelper is IFlashLoanRecipient {
             transfers, execution.borrowerCaveat, execution.lenderCaveat, execution.loan
         );
     }
+
+    function setFlashVault(address vault) external onlyOwner {
+        VAULT = vault;
+    }
 }

src/Custodian.sol

@@ -366,7 +366,9 @@ contract Custodian is ERC721, ContractOffererInterface {
         } else if (offer.itemType == ItemType.ERC1155) {
             ERC1155(offer.token).setApprovalForAll(seaport, true);
         } else if (offer.itemType == ItemType.ERC20) {
-            SafeTransferLib.safeApproveWithRetry(offer.token, seaport, type(uint256).max);
+            if (ERC20(offer.token).allowance(address(this), seaport) != type(uint256).max) {
+                SafeTransferLib.safeApproveWithRetry(offer.token, seaport, type(uint256).max);
+            }
         }
     }
 
@@ -376,8 +378,12 @@ contract Custodian is ERC721, ContractOffererInterface {
      */
     function _setOfferApprovalsWithSeaport(Starport.Loan memory loan) internal {
         _beforeApprovalsSetHook(loan);
-        for (uint256 i = 0; i < loan.collateral.length; i++) {
+        uint256 i = 0;
+        for (; i < loan.collateral.length;) {
             _enableAssetWithSeaport(loan.collateral[i]);
+            unchecked {
+                ++i;
+            }
         }
     }
 
@@ -403,8 +409,12 @@ contract Custodian is ERC721, ContractOffererInterface {
      * @param authorized The address handling the asset further
      */
     function _moveCollateralToAuthorized(SpentItem[] memory offer, address authorized) internal {
-        for (uint256 i = 0; i < offer.length; i++) {
+        uint256 i = 0;
+        for (; i < offer.length;) {
             _transferCollateralAuthorized(offer[i], authorized);
+            unchecked {
+                ++i;
+            }
         }
     }
 

src/Starport.sol

@@ -317,9 +317,11 @@ contract Starport is PausableNonReentrant {
      * @dev Increments caveat nonce for sender and emits event
      */
     function incrementCaveatNonce() external {
-        uint256 newNonce = caveatNonces[msg.sender] + 1 + uint256(blockhash(block.number - 1) >> 0x80);
-        caveatNonces[msg.sender] = newNonce;
-        emit CaveatNonceIncremented(msg.sender, newNonce);
+        unchecked {
+            uint256 newNonce = caveatNonces[msg.sender] + 1 + uint256(blockhash(block.number - 1) >> 0x80);
+            caveatNonces[msg.sender] = newNonce;
+            emit CaveatNonceIncremented(msg.sender, newNonce);
+        }
     }
 
     /**

src/originators/StrategistOriginator.sol

@@ -147,7 +147,9 @@ contract StrategistOriginator is Ownable, Originator, TokenReceiverInterface {
         if (msg.sender != strategist && msg.sender != owner()) {
             revert NotAuthorized();
         }
-        _counter += 1 + uint256(blockhash(block.number - 1) >> 0x80);
+        unchecked {
+            _counter += 1 + uint256(blockhash(block.number - 1) >> 0x80);
+        }
         emit CounterUpdated(_counter);
     }
 
@@ -223,7 +225,8 @@ contract StrategistOriginator is Ownable, Originator, TokenReceiverInterface {
         }
 
         // Loop through collateral and check if the collateral is the same
-        for (uint256 i = 0; i < request.debt.length;) {
+        uint256 i = 0;
+        for (; i < request.debt.length;) {
             if (
                 request.debt[i].itemType != details.offer.debt[i].itemType
                     || request.debt[i].token != details.offer.debt[i].token
@@ -239,7 +242,7 @@ contract StrategistOriginator is Ownable, Originator, TokenReceiverInterface {
                 revert InvalidDebtAmount();
             }
             unchecked {
-                i++;
+                ++i;
             }
         }
     }

src/settlement/DutchAuctionSettlement.sol

@@ -143,14 +143,17 @@ abstract contract DutchAuctionSettlement is Settlement, AmountDeriver {
 
         if (carry > 0 && loan.debt[0].amount + interest - carry < settlementPrice) {
             consideration = new ReceivedItem[](2);
-            uint256 excess = settlementPrice - (loan.debt[0].amount + interest - carry);
-            consideration[0] = ReceivedItem({
-                itemType: loan.debt[0].itemType,
-                identifier: loan.debt[0].identifier,
-                amount: (excess > carry) ? carry : excess,
-                token: loan.debt[0].token,
-                recipient: payable(loan.originator)
-            });
+            unchecked {
+                uint256 excess = settlementPrice - (loan.debt[0].amount + interest - carry);
+
+                consideration[0] = ReceivedItem({
+                    itemType: loan.debt[0].itemType,
+                    identifier: loan.debt[0].identifier,
+                    amount: (excess > carry) ? carry : excess,
+                    token: loan.debt[0].token,
+                    recipient: payable(loan.originator)
+                });
+            }
             settlementPrice -= consideration[0].amount;
         } else {
             consideration = new ReceivedItem[](1);

test/integration-testing/TestNewLoan.sol

@@ -176,7 +176,7 @@ contract TestNewLoan is StarportTest {
         loan.debt[0].identifier = 0;
         loan.debt[0].amount = 100;
 
-        BNPLHelper helper = new BNPLHelper();
+        BNPLHelper helper = new BNPLHelper(address(0xBA12222222228d8Ba445958a75a0704d566BF2C8), address(this));
 
         AdvancedOrder[] memory orders = new AdvancedOrder[](2);
         orders[0] = AdvancedOrder({
@@ -273,20 +273,6 @@ contract TestNewLoan is StarportTest {
         }
     }
 
-    function testInvalidUserDataHashBNPL() public {
-        BNPLHelper helper = new BNPLHelper();
-
-        vm.prank(address(0xBA12222222228d8Ba445958a75a0704d566BF2C8)); //the vault address for balancer
-        vm.expectRevert(abi.encodeWithSelector(BNPLHelper.InvalidUserDataProvided.selector));
-        helper.receiveFlashLoan(new address[](0), new uint256[](0), new uint256[](0), bytes(""));
-    }
-
-    function testInvalidSenderBNPL() public {
-        BNPLHelper helper = new BNPLHelper();
-        vm.expectRevert(abi.encodeWithSelector(BNPLHelper.SenderNotVault.selector));
-        helper.receiveFlashLoan(new address[](0), new uint256[](0), new uint256[](0), bytes(""));
-    }
-
     function testNewLoanViaOriginatorLenderApproval() public {
         Starport.Loan memory loan = generateDefaultLoanTerms();