From a6aaccb9a6231fedcecf51368cb83114bdbbe3d7 Mon Sep 17 00:00:00 2001 From: Joseph Delong Date: Thu, 18 Jan 2024 18:14:22 -0600 Subject: [PATCH] Fix/audit fixes round 7 (#93) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 | 66 +++++++++++------------ package.json | 3 +- src/BNPLHelper.sol | 26 +++++---- src/Custodian.sol | 16 ++++-- src/Starport.sol | 8 +-- src/originators/StrategistOriginator.sol | 9 ++-- src/settlement/DutchAuctionSettlement.sol | 19 ++++--- test/integration-testing/TestNewLoan.sol | 16 +----- 8 files changed, 82 insertions(+), 81 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 5e00f50e..1e79de56 100644 --- a/.gas-snapshot +++ b/.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) diff --git a/package.json b/package.json index f1f3168f..077cdbdb 100644 --- a/package.json +++ b/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", diff --git a/src/BNPLHelper.sol b/src/BNPLHelper.sol index 5e3a5bf9..21e63c42 100644 --- a/src/BNPLHelper.sol +++ b/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; + } } diff --git a/src/Custodian.sol b/src/Custodian.sol index ab01c97a..9da68f90 100644 --- a/src/Custodian.sol +++ b/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; + } } } diff --git a/src/Starport.sol b/src/Starport.sol index ad342b64..46aa29ec 100644 --- a/src/Starport.sol +++ b/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); + } } /** diff --git a/src/originators/StrategistOriginator.sol b/src/originators/StrategistOriginator.sol index ac88fb34..ea388978 100644 --- a/src/originators/StrategistOriginator.sol +++ b/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; } } } diff --git a/src/settlement/DutchAuctionSettlement.sol b/src/settlement/DutchAuctionSettlement.sol index e8d4a9d6..2835f614 100644 --- a/src/settlement/DutchAuctionSettlement.sol +++ b/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); diff --git a/test/integration-testing/TestNewLoan.sol b/test/integration-testing/TestNewLoan.sol index 1f83f90b..286ad142 100644 --- a/test/integration-testing/TestNewLoan.sol +++ b/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();