From bdaf5c9b61f5444523f641d16c30f2edd011ba3e Mon Sep 17 00:00:00 2001 From: fabiohild Date: Thu, 8 Feb 2024 22:08:22 -0300 Subject: [PATCH 1/4] adding codesize validation instead of null address --- src/contracts/AccountingEngine.sol | 4 ++-- src/contracts/CollateralAuctionHouse.sol | 4 ++-- src/contracts/DebtAuctionHouse.sol | 2 +- src/contracts/LiquidationEngine.sol | 4 ++-- src/contracts/OracleRelayer.sol | 8 ++++---- src/contracts/PIDRateSetter.sol | 4 ++-- src/contracts/SurplusAuctionHouse.sol | 2 +- src/contracts/settlement/GlobalSettlement.sol | 14 +++++++------- .../PostSettlementSurplusAuctionHouse.sol | 2 +- src/libraries/Assertions.sol | 8 ++++++++ test/testnet/unit/AccountingEngine.t.sol | 6 ++++-- test/testnet/unit/CollateralAuctionHouse.t.sol | 12 ++++++++---- test/testnet/unit/DebtAuctionHouse.t.sol | 6 ++++-- test/testnet/unit/GlobalSettlement.t.sol | 13 +++++++++++++ test/testnet/unit/LiquidationEngine.t.sol | 14 ++++++++++++-- test/testnet/unit/OracleRelayer.t.sol | 14 ++++++++------ test/testnet/unit/PIDRateSetter.t.sol | 6 ++++-- .../unit/PostSettlementSurplusAuctionHouse.t.sol | 6 ++++-- test/testnet/unit/SurplusAuctionHouse.t.sol | 6 ++++-- 19 files changed, 91 insertions(+), 44 deletions(-) diff --git a/src/contracts/AccountingEngine.sol b/src/contracts/AccountingEngine.sol index 078dee5c..3c11efa1 100644 --- a/src/contracts/AccountingEngine.sol +++ b/src/contracts/AccountingEngine.sol @@ -313,7 +313,7 @@ contract AccountingEngine is Authorizable, Modifiable, Disableable, IAccountingE /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(surplusAuctionHouse).assertNonNull(); - address(debtAuctionHouse).assertNonNull(); + address(surplusAuctionHouse).assertHasCode(); + address(debtAuctionHouse).assertHasCode(); } } diff --git a/src/contracts/CollateralAuctionHouse.sol b/src/contracts/CollateralAuctionHouse.sol index 625f3679..88f9e74e 100644 --- a/src/contracts/CollateralAuctionHouse.sol +++ b/src/contracts/CollateralAuctionHouse.sol @@ -399,8 +399,8 @@ contract CollateralAuctionHouse is Authorizable, Modifiable, Disableable, IColla /// @inheritdoc Modifiable function _validateParameters() internal view override { // Registry - address(liquidationEngine()).assertNonNull(); - address(oracleRelayer()).assertNonNull(); + address(liquidationEngine()).assertHasCode(); + address(oracleRelayer()).assertHasCode(); // CAH Params _params.minDiscount.assertGtEq(_params.maxDiscount).assertLtEq(WAD); _params.maxDiscount.assertGt(0); diff --git a/src/contracts/DebtAuctionHouse.sol b/src/contracts/DebtAuctionHouse.sol index 4aac3ac2..ac309617 100644 --- a/src/contracts/DebtAuctionHouse.sol +++ b/src/contracts/DebtAuctionHouse.sol @@ -223,6 +223,6 @@ contract DebtAuctionHouse is Authorizable, Modifiable, Disableable, IDebtAuction /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(protocolToken).assertNonNull(); + address(protocolToken).assertHasCode(); } } diff --git a/src/contracts/LiquidationEngine.sol b/src/contracts/LiquidationEngine.sol index 625a5f4a..21358217 100644 --- a/src/contracts/LiquidationEngine.sol +++ b/src/contracts/LiquidationEngine.sol @@ -287,13 +287,13 @@ contract LiquidationEngine is Authorizable, Modifiable, Disableable, ReentrancyG /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(accountingEngine).assertNonNull(); + address(accountingEngine).assertHasCode(); } /// @inheritdoc Modifiable function _validateCParameters(bytes32 _cType) internal view override { LiquidationEngineCollateralParams memory __cParams = _cParams[_cType]; - address(__cParams.collateralAuctionHouse).assertNonNull(); + address(__cParams.collateralAuctionHouse).assertHasCode(); __cParams.liquidationQuantity.assertLtEq(MAX_RAD); } diff --git a/src/contracts/OracleRelayer.sol b/src/contracts/OracleRelayer.sol index 6e45b180..d370defc 100644 --- a/src/contracts/OracleRelayer.sol +++ b/src/contracts/OracleRelayer.sol @@ -178,7 +178,7 @@ contract OracleRelayer is Authorizable, Modifiable, Disableable, IOracleRelayer function _modifyParameters(bytes32 _param, bytes memory _data) internal override whenEnabled { uint256 _uint256 = _data.toUint256(); - if (_param == 'systemCoinOracle') systemCoinOracle = IBaseOracle(_data.toAddress().assertNonNull()); + if (_param == 'systemCoinOracle') systemCoinOracle = IBaseOracle(_data.toAddress().assertHasCode()); else if (_param == 'redemptionRateUpperBound') _params.redemptionRateUpperBound = _uint256; else if (_param == 'redemptionRateLowerBound') _params.redemptionRateLowerBound = _uint256; else revert UnrecognizedParam(); @@ -199,7 +199,7 @@ contract OracleRelayer is Authorizable, Modifiable, Disableable, IOracleRelayer /// @dev Validates the address is IDelayedOracle compliant and returns it function _validateDelayedOracle(address _oracle) internal view returns (IDelayedOracle _delayedOracle) { // Checks if the delayed oracle priceSource is implemented - _delayedOracle = IDelayedOracle(_oracle.assertNonNull()); + _delayedOracle = IDelayedOracle(_oracle.assertHasCode()); _delayedOracle.priceSource(); } @@ -207,7 +207,7 @@ contract OracleRelayer is Authorizable, Modifiable, Disableable, IOracleRelayer function _validateParameters() internal view override { _params.redemptionRateUpperBound.assertGt(RAY); _params.redemptionRateLowerBound.assertGt(0).assertLt(RAY); - address(systemCoinOracle).assertNonNull(); + address(systemCoinOracle).assertHasCode(); } /// @inheritdoc Modifiable @@ -215,6 +215,6 @@ contract OracleRelayer is Authorizable, Modifiable, Disableable, IOracleRelayer OracleRelayerCollateralParams memory __cParams = _cParams[_cType]; __cParams.safetyCRatio.assertGtEq(__cParams.liquidationCRatio); __cParams.liquidationCRatio.assertGtEq(RAY); - address(__cParams.oracle).assertNonNull(); + address(__cParams.oracle).assertHasCode(); } } diff --git a/src/contracts/PIDRateSetter.sol b/src/contracts/PIDRateSetter.sol index f386f9a3..3328eb1a 100644 --- a/src/contracts/PIDRateSetter.sol +++ b/src/contracts/PIDRateSetter.sol @@ -101,7 +101,7 @@ contract PIDRateSetter is Authorizable, Modifiable, IPIDRateSetter { function _validateParameters() internal view override { _params.updateRateDelay.assertGt(0); - address(oracleRelayer).assertNonNull(); - address(pidCalculator).assertNonNull(); + address(oracleRelayer).assertHasCode(); + address(pidCalculator).assertHasCode(); } } diff --git a/src/contracts/SurplusAuctionHouse.sol b/src/contracts/SurplusAuctionHouse.sol index fff28f1f..25e7348a 100644 --- a/src/contracts/SurplusAuctionHouse.sol +++ b/src/contracts/SurplusAuctionHouse.sol @@ -224,7 +224,7 @@ contract SurplusAuctionHouse is Authorizable, Modifiable, Disableable, ISurplusA /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(protocolToken).assertNonNull(); + address(protocolToken).assertHasCode(); _params.bidReceiver.assertNonNull(); } } diff --git a/src/contracts/settlement/GlobalSettlement.sol b/src/contracts/settlement/GlobalSettlement.sol index f2330b4a..d665971b 100644 --- a/src/contracts/settlement/GlobalSettlement.sol +++ b/src/contracts/settlement/GlobalSettlement.sol @@ -375,12 +375,12 @@ contract GlobalSettlement is Authorizable, Modifiable, Disableable, IGlobalSettl /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(liquidationEngine).assertNonNull(); - address(oracleRelayer).assertNonNull(); - address(coinJoin).assertNonNull(); - address(collateralJoinFactory).assertNonNull(); - address(collateralAuctionHouseFactory).assertNonNull(); - address(stabilityFeeTreasury).assertNonNull(); - address(accountingEngine).assertNonNull(); + address(liquidationEngine).assertHasCode(); + address(oracleRelayer).assertHasCode(); + address(coinJoin).assertHasCode(); + address(collateralJoinFactory).assertHasCode(); + address(collateralAuctionHouseFactory).assertHasCode(); + address(stabilityFeeTreasury).assertHasCode(); + address(accountingEngine).assertHasCode(); } } diff --git a/src/contracts/settlement/PostSettlementSurplusAuctionHouse.sol b/src/contracts/settlement/PostSettlementSurplusAuctionHouse.sol index 65dece32..fbfdc778 100644 --- a/src/contracts/settlement/PostSettlementSurplusAuctionHouse.sol +++ b/src/contracts/settlement/PostSettlementSurplusAuctionHouse.sol @@ -167,6 +167,6 @@ contract PostSettlementSurplusAuctionHouse is Authorizable, Modifiable, IPostSet /// @inheritdoc Modifiable function _validateParameters() internal view override { - address(protocolToken).assertNonNull(); + address(protocolToken).assertHasCode(); } } diff --git a/src/libraries/Assertions.sol b/src/libraries/Assertions.sol index 2929e88f..e0ffee0c 100644 --- a/src/libraries/Assertions.sol +++ b/src/libraries/Assertions.sol @@ -28,6 +28,8 @@ library Assertions { error NullAmount(); /// @dev Throws if checked address is null error NullAddress(); + /// @dev Throws if checked address contains no code + error NoCode(address _contract); // --- Assertions --- @@ -90,4 +92,10 @@ library Assertions { if (_address == address(0)) revert NullAddress(); return _address; } + + /// @dev Asserts that `_address` contains code and returns `_address` + function assertHasCode(address _address) internal view returns (address __address) { + if (_address.code.length == 0) revert NoCode(_address); + return _address; + } } diff --git a/test/testnet/unit/AccountingEngine.t.sol b/test/testnet/unit/AccountingEngine.t.sol index 6c4cb885..ee550785 100644 --- a/test/testnet/unit/AccountingEngine.t.sol +++ b/test/testnet/unit/AccountingEngine.t.sol @@ -220,14 +220,14 @@ contract Unit_AccountingEngine_Constructor is Base { } function test_Revert_NullSurplusAuctionHouse() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new AccountingEngineForTest( address(mockSafeEngine), address(0), address(mockDebtAuctionHouse), accountingEngineParams ); } function test_Revert_NullDebtAuctionHouse() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new AccountingEngineForTest( address(mockSafeEngine), address(mockSurplusAuctionHouse), address(0), accountingEngineParams ); @@ -252,6 +252,7 @@ contract Unit_AccountingEngine_ModifyParameters is Base { function test_ModifyParameters_SurplusAuctionHouse(address _surplusAuctionHouse) public authorized { vm.assume(_surplusAuctionHouse != address(0)); + vm.etch(_surplusAuctionHouse, '0xF'); address _previousSurplusAuctionHouse = address(accountingEngine.surplusAuctionHouse()); if (_previousSurplusAuctionHouse != address(0)) { vm.expectCall( @@ -271,6 +272,7 @@ contract Unit_AccountingEngine_ModifyParameters is Base { function test_ModifyParameters_DebtAuctionHouse(address _debtAuctionHouse) public authorized { vm.assume(_debtAuctionHouse != address(0)); + vm.etch(_debtAuctionHouse, '0xF'); accountingEngine.modifyParameters('debtAuctionHouse', abi.encode(_debtAuctionHouse)); assertEq(_debtAuctionHouse, address(accountingEngine.debtAuctionHouse())); diff --git a/test/testnet/unit/CollateralAuctionHouse.t.sol b/test/testnet/unit/CollateralAuctionHouse.t.sol index 34b8e740..f903d859 100644 --- a/test/testnet/unit/CollateralAuctionHouse.t.sol +++ b/test/testnet/unit/CollateralAuctionHouse.t.sol @@ -217,7 +217,7 @@ contract Unit_CollateralAuctionHouse_Constructor is Base { } function test_Revert_NullAddress_LiquidationEngine() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new CollateralAuctionHouseForTest( address(mockSafeEngine), address(0), address(mockOracleRelayer), collateralType, cahParams @@ -225,7 +225,7 @@ contract Unit_CollateralAuctionHouse_Constructor is Base { } function test_Revert_NullAddress_OracleRelayer() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new CollateralAuctionHouseForTest( address(mockSafeEngine), address(mockLiquidationEngine), address(0), collateralType, cahParams @@ -1611,6 +1611,8 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { vm.assume(_liquidationEngine != deployer); vm.assume(_liquidationEngine != authorizedAccount); + vm.etch(_liquidationEngine, '0xF'); + collateralAuctionHouse.modifyParameters('liquidationEngine', abi.encode(_liquidationEngine)); assertEq(address(collateralAuctionHouse.liquidationEngine()), _liquidationEngine); @@ -1620,6 +1622,7 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { address _oldLiquidationEngine, address _newLiquidationEngine ) public happyPath { + vm.etch(_newLiquidationEngine, '0xF'); vm.assume(_newLiquidationEngine != address(0)); vm.assume(_newLiquidationEngine != deployer); vm.assume(_newLiquidationEngine != authorizedAccount); @@ -1642,6 +1645,7 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { vm.assume(_oracleRelayer != address(0)); + vm.etch(_oracleRelayer, '0xF'); collateralAuctionHouse.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); @@ -1651,7 +1655,7 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { function test_Revert_NullAddress_LiquidationEngine() public { vm.startPrank(authorizedAccount); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); collateralAuctionHouse.modifyParameters('liquidationEngine', abi.encode(0)); } @@ -1659,7 +1663,7 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { function test_Revert_NullAddress_OracleRelayer() public { vm.startPrank(authorizedAccount); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); collateralAuctionHouse.modifyParameters('oracleRelayer', abi.encode(0)); } diff --git a/test/testnet/unit/DebtAuctionHouse.t.sol b/test/testnet/unit/DebtAuctionHouse.t.sol index 0498acc5..0d670fa1 100644 --- a/test/testnet/unit/DebtAuctionHouse.t.sol +++ b/test/testnet/unit/DebtAuctionHouse.t.sol @@ -159,6 +159,7 @@ contract Unit_DebtAuctionHouse_Constructor is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); debtAuctionHouse = new DebtAuctionHouseForTest(address(mockSafeEngine), _protocolToken, dahParams); assertEq(address(debtAuctionHouse.protocolToken()), _protocolToken); @@ -177,7 +178,7 @@ contract Unit_DebtAuctionHouse_Constructor is Base { } function test_Revert_Null_ProtocolToken() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new DebtAuctionHouseForTest(address(mockSafeEngine), address(0), dahParams); } @@ -931,6 +932,7 @@ contract Unit_DebtAuctionHouse_ModifyParameters is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); debtAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(debtAuctionHouse.protocolToken()), _protocolToken); @@ -938,7 +940,7 @@ contract Unit_DebtAuctionHouse_ModifyParameters is Base { function test_Revert_ProtocolToken_NullAddress() public { vm.startPrank(authorizedAccount); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); debtAuctionHouse.modifyParameters('protocolToken', abi.encode(0)); } diff --git a/test/testnet/unit/GlobalSettlement.t.sol b/test/testnet/unit/GlobalSettlement.t.sol index aed5ed84..bd21cd41 100644 --- a/test/testnet/unit/GlobalSettlement.t.sol +++ b/test/testnet/unit/GlobalSettlement.t.sol @@ -244,6 +244,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { vm.assume(address(_liquidationEngine) != address(0)); + vm.etch(_liquidationEngine, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(_liquidationEngine), @@ -260,6 +261,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { vm.assume(address(_oracleRelayer) != address(0)); + vm.etch(_oracleRelayer, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -276,6 +278,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_CoinJoin(address _coinJoin) public happyPath { vm.assume(address(_coinJoin) != address(0)); + vm.etch(_coinJoin, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -292,6 +295,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath { vm.assume(address(_collateralJoinFactory) != address(0)); + vm.etch(_collateralJoinFactory, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -308,6 +312,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_CollateralAuctionHouseFactory(address _collateralAuctionHouseFactory) public happyPath { vm.assume(address(_collateralAuctionHouseFactory) != address(0)); + vm.etch(_collateralAuctionHouseFactory, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -324,6 +329,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_StabilityFeeTreasury(address _sfTreasury) public happyPath { vm.assume(address(_sfTreasury) != address(0)); + vm.etch(_sfTreasury, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -340,6 +346,7 @@ contract Unit_GlobalSettlement_Constructor is Base { function test_Set_AccountingEngine(address _accountingEngine) public happyPath { vm.assume(address(_accountingEngine) != address(0)); + vm.etch(_accountingEngine, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -1282,6 +1289,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { vm.assume(_liquidationEngine != address(0)); + vm.etch(_liquidationEngine, '0xF'); globalSettlement.modifyParameters('liquidationEngine', abi.encode(_liquidationEngine)); assertEq(address(globalSettlement.liquidationEngine()), _liquidationEngine); @@ -1289,6 +1297,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_AccountingEngine(address _accountingEngine) public happyPath { vm.assume(_accountingEngine != address(0)); + vm.etch(_accountingEngine, '0xF'); globalSettlement.modifyParameters('accountingEngine', abi.encode(_accountingEngine)); assertEq(address(globalSettlement.accountingEngine()), _accountingEngine); @@ -1296,6 +1305,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { vm.assume(_oracleRelayer != address(0)); + vm.etch(_oracleRelayer, '0xF'); globalSettlement.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); assertEq(address(globalSettlement.oracleRelayer()), _oracleRelayer); @@ -1303,6 +1313,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_CoinJoin(address _coinJoin) public happyPath { vm.assume(_coinJoin != address(0)); + vm.etch(_coinJoin, '0xF'); globalSettlement.modifyParameters('coinJoin', abi.encode(_coinJoin)); assertEq(address(globalSettlement.coinJoin()), _coinJoin); @@ -1310,6 +1321,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath { vm.assume(_collateralJoinFactory != address(0)); + vm.etch(_collateralJoinFactory, '0xF'); globalSettlement.modifyParameters('collateralJoinFactory', abi.encode(_collateralJoinFactory)); assertEq(address(globalSettlement.collateralJoinFactory()), _collateralJoinFactory); @@ -1317,6 +1329,7 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath { vm.assume(_stabilityFeeTreasury != address(0)); + vm.etch(_stabilityFeeTreasury, '0xF'); globalSettlement.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(globalSettlement.stabilityFeeTreasury()), _stabilityFeeTreasury); diff --git a/test/testnet/unit/LiquidationEngine.t.sol b/test/testnet/unit/LiquidationEngine.t.sol index f07aa6a5..3c24ac36 100644 --- a/test/testnet/unit/LiquidationEngine.t.sol +++ b/test/testnet/unit/LiquidationEngine.t.sol @@ -315,7 +315,7 @@ contract Unit_LiquidationEngine_Constructor is Base { } function test_Revert_Null_AccountingEngine() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new LiquidationEngine(address(mockSafeEngine), address(0), liquidationEngineParams); } @@ -336,6 +336,8 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { ) public authorized { _mockCollateralList(_cType); + vm.etch(_fuzz.collateralAuctionHouse, '0xF'); + vm.assume(_fuzz.collateralAuctionHouse != address(0)); vm.assume(_fuzz.collateralAuctionHouse != deployer); liquidationEngine.modifyParameters(_cType, 'collateralAuctionHouse', abi.encode(_fuzz.collateralAuctionHouse)); @@ -364,6 +366,7 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { function test_ModifyParameters_AccountingEngine(address _accountingEngine) public authorized { vm.assume(_accountingEngine != address(0)); + vm.etch(_accountingEngine, '0xF'); liquidationEngine.modifyParameters('accountingEngine', abi.encode(_accountingEngine)); assertEq(_accountingEngine, address(liquidationEngine.accountingEngine())); @@ -380,6 +383,8 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { vm.assume(_newCAH != deployer); vm.assume(_previousCAH != deployer); + vm.etch(_newCAH, '0xF'); + LiquidationEngineForTest(address(liquidationEngine)).setCollateralAuctionHouse(_cType, _previousCAH); if (_previousCAH != address(0)) { @@ -1801,6 +1806,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams ) public authorized happyPath(_liqEngineCParams) { + vm.etch(address(_liqEngineCParams.collateralAuctionHouse), '0xF'); liquidationEngine.initializeCollateralType(_cType, _liqEngineCParams); assertEq(abi.encode(liquidationEngine.cParams(_cType)), abi.encode(_liqEngineCParams)); @@ -1810,6 +1816,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams ) public authorized happyPath(_liqEngineCParams) { + vm.etch(_liqEngineCParams.collateralAuctionHouse, '0xF'); vm.expectCall( address(mockSafeEngine), abi.encodeCall(mockSafeEngine.approveSAFEModification, (_liqEngineCParams.collateralAuctionHouse)) @@ -1822,6 +1829,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams ) public authorized happyPath(_liqEngineCParams) { + vm.etch(_liqEngineCParams.collateralAuctionHouse, '0xF'); vm.expectEmit(); emit AddAuthorization(_liqEngineCParams.collateralAuctionHouse); @@ -1834,7 +1842,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { ) public authorized { _liqEngineCParams.collateralAuctionHouse = address(0); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); liquidationEngine.initializeCollateralType(_cType, _liqEngineCParams); } @@ -1847,6 +1855,8 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { vm.assume(_liqEngineCParams.collateralAuctionHouse != deployer); vm.assume(_liqEngineCParams.liquidationQuantity > MAX_RAD); + vm.etch(address(_liqEngineCParams.collateralAuctionHouse), '0xF'); + vm.expectRevert( abi.encodeWithSelector(Assertions.NotLesserOrEqualThan.selector, _liqEngineCParams.liquidationQuantity, MAX_RAD) ); diff --git a/test/testnet/unit/OracleRelayer.t.sol b/test/testnet/unit/OracleRelayer.t.sol index 14fb0c77..03e1d33c 100644 --- a/test/testnet/unit/OracleRelayer.t.sol +++ b/test/testnet/unit/OracleRelayer.t.sol @@ -145,7 +145,7 @@ contract Unit_OracleRelayer_Constructor is Base { } function test_Revert_Null_SystemCoinOracle() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new OracleRelayer(address(mockSafeEngine), IBaseOracle(address(0)), oracleRelayerParams); } @@ -205,13 +205,15 @@ contract Unit_OracleRelayer_ModifyParameters is Base { // NOTE: needs to have a valid liqCRatio to pass the `modifyParameters` check _mockCTypeLiquidationCRatio(_cType, 1e27); - oracleRelayer.modifyParameters(_cType, 'safetyCRatio', abi.encode(_fuzz.safetyCRatio)); - oracleRelayer.modifyParameters(_cType, 'liquidationCRatio', abi.encode(_fuzz.liquidationCRatio)); - + vm.etch(_fuzzPriceSource, '0xF'); vm.mockCall( address(_fuzz.oracle), abi.encodeWithSelector(IDelayedOracle.priceSource.selector), abi.encode(_fuzzPriceSource) ); + vm.etch(address(_fuzz.oracle), '0xF'); + oracleRelayer.modifyParameters(_cType, 'oracle', abi.encode(_fuzz.oracle)); + oracleRelayer.modifyParameters(_cType, 'safetyCRatio', abi.encode(_fuzz.safetyCRatio)); + oracleRelayer.modifyParameters(_cType, 'liquidationCRatio', abi.encode(_fuzz.liquidationCRatio)); IOracleRelayer.OracleRelayerCollateralParams memory _cParams = oracleRelayer.cParams(_cType); @@ -304,7 +306,7 @@ contract Unit_OracleRelayer_ModifyParameters is Base { authorized previousValidCTypeParams(_cType) { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); oracleRelayer.modifyParameters(_cType, 'oracle', abi.encode(address(0))); } @@ -869,7 +871,7 @@ contract Unit_OracleRelayer_InitializeCollateralType is Base { ) public authorized { _oracleRelayerCParams.oracle = IDelayedOracle(address(0)); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); oracleRelayer.initializeCollateralType(_cType, _oracleRelayerCParams); } diff --git a/test/testnet/unit/PIDRateSetter.t.sol b/test/testnet/unit/PIDRateSetter.t.sol index 8fc29fb1..c9e0c0e1 100644 --- a/test/testnet/unit/PIDRateSetter.t.sol +++ b/test/testnet/unit/PIDRateSetter.t.sol @@ -101,12 +101,12 @@ contract Unit_PIDRateSetter_Constructor is Base { } function test_Revert_NullOracleRelayerAddress() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new PIDRateSetter(address(0), address(mockPIDController), IPIDRateSetter.PIDRateSetterParams(periodSize)); } function test_Revert_NullCalculator() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new PIDRateSetter(address(mockOracleRelayer), address(0), IPIDRateSetter.PIDRateSetterParams(periodSize)); } } @@ -123,6 +123,7 @@ contract Unit_PIDRateSetter_ModifyParameters is Base { function test_ModifyParameters_Set_OracleRelayer(address _oracleRelayer) public authorized { vm.assume(_oracleRelayer != address(0)); + vm.etch(_oracleRelayer, '0xF'); pidRateSetter.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); assertEq(address(pidRateSetter.oracleRelayer()), _oracleRelayer); @@ -130,6 +131,7 @@ contract Unit_PIDRateSetter_ModifyParameters is Base { function test_ModifyParameters_Set_PIDCalculator(address _pidCalculator) public authorized { vm.assume(_pidCalculator != address(0)); + vm.etch(_pidCalculator, '0xF'); pidRateSetter.modifyParameters('pidCalculator', abi.encode(_pidCalculator)); assertEq(address(pidRateSetter.pidCalculator()), _pidCalculator); diff --git a/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol b/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol index 61d870bc..6c01bc2e 100644 --- a/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol +++ b/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol @@ -116,6 +116,7 @@ contract Unit_PostSettlementSurplusAuctionHouse_Constructor is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); postSettlementSurplusAuctionHouse = new PostSettlementSurplusAuctionHouseForTest(address(mockSafeEngine), _protocolToken, pssahParams); @@ -139,7 +140,7 @@ contract Unit_PostSettlementSurplusAuctionHouse_Constructor is Base { } function test_Revert_Null_ProtocolToken() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new PostSettlementSurplusAuctionHouseForTest(address(mockSafeEngine), address(0), pssahParams); } @@ -681,6 +682,7 @@ contract Unit_PostSettlementSurplusAuctionHouse_ModifyParameters is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); postSettlementSurplusAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(postSettlementSurplusAuctionHouse.protocolToken()), _protocolToken); @@ -688,7 +690,7 @@ contract Unit_PostSettlementSurplusAuctionHouse_ModifyParameters is Base { function test_Revert_ProtocolToken_NullAddress() public { vm.startPrank(authorizedAccount); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); postSettlementSurplusAuctionHouse.modifyParameters('protocolToken', abi.encode(0)); } diff --git a/test/testnet/unit/SurplusAuctionHouse.t.sol b/test/testnet/unit/SurplusAuctionHouse.t.sol index 8dadd4fe..d4b81abe 100644 --- a/test/testnet/unit/SurplusAuctionHouse.t.sol +++ b/test/testnet/unit/SurplusAuctionHouse.t.sol @@ -149,6 +149,7 @@ contract Unit_SurplusAuctionHouse_Constructor is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); surplusAuctionHouse = new SurplusAuctionHouseForTest(address(mockSafeEngine), _protocolToken, sahParams); assertEq(address(surplusAuctionHouse.protocolToken()), _protocolToken); @@ -169,7 +170,7 @@ contract Unit_SurplusAuctionHouse_Constructor is Base { } function test_Revert_Null_ProtocolToken() public { - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); new SurplusAuctionHouseForTest(address(mockSafeEngine), address(0), sahParams); } @@ -917,6 +918,7 @@ contract Unit_SurplusAuctionHouse_ModifyParameters is Base { function test_Set_ProtocolToken(address _protocolToken) public happyPath { vm.assume(_protocolToken != address(0)); + vm.etch(_protocolToken, '0xF'); surplusAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(surplusAuctionHouse.protocolToken()), _protocolToken); @@ -924,7 +926,7 @@ contract Unit_SurplusAuctionHouse_ModifyParameters is Base { function test_Revert_ProtocolToken_NullAddress() public { vm.startPrank(authorizedAccount); - vm.expectRevert(Assertions.NullAddress.selector); + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); surplusAuctionHouse.modifyParameters('protocolToken', abi.encode(0)); } From 808dcaa67f99e8ca9c2989f3bb8444d5e1b5b5f3 Mon Sep 17 00:00:00 2001 From: 0xJabberwock <102967621+0xJabberwock@users.noreply.github.com> Date: Fri, 27 Oct 2023 08:19:11 -0300 Subject: [PATCH 2/4] fix: jobs parameters validation * fix: jobs parameters validation * feat: job is modifiable --- src/contracts/jobs/AccountingJob.sol | 22 +++-- src/contracts/jobs/Job.sol | 27 +++++- src/contracts/jobs/LiquidationJob.sol | 23 +++--- src/contracts/jobs/OracleJob.sol | 20 +++-- src/interfaces/jobs/IAccountingJob.sol | 4 +- src/interfaces/jobs/IJob.sol | 5 +- src/interfaces/jobs/ILiquidationJob.sol | 2 +- src/interfaces/jobs/IOracleJob.sol | 4 +- test/testnet/mocks/JobForTest.sol | 6 +- test/testnet/unit/jobs/AccountingJob.t.sol | 73 +++++++++++++--- test/testnet/unit/jobs/Job.t.sol | 91 ++++++++++++++++++-- test/testnet/unit/jobs/LiquidationJob.t.sol | 65 ++++++++++++--- test/testnet/unit/jobs/OracleJob.t.sol | 92 +++++++++++++++++---- 13 files changed, 355 insertions(+), 79 deletions(-) diff --git a/src/contracts/jobs/AccountingJob.sol b/src/contracts/jobs/AccountingJob.sol index 38293f06..5a2ec43b 100644 --- a/src/contracts/jobs/AccountingJob.sol +++ b/src/contracts/jobs/AccountingJob.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.19; import {IAccountingJob} from '@interfaces/jobs/IAccountingJob.sol'; import {IAccountingEngine} from '@interfaces/IAccountingEngine.sol'; -import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; import {Job} from '@contracts/jobs/Job.sol'; @@ -11,13 +10,15 @@ import {Authorizable} from '@contracts/utils/Authorizable.sol'; import {Modifiable} from '@contracts/utils/Modifiable.sol'; import {Encoding} from '@libraries/Encoding.sol'; +import {Assertions} from '@libraries/Assertions.sol'; /** * @title AccountingJob * @notice This contract contains rewarded methods to handle the accounting engine debt and surplus */ -contract AccountingJob is Job, Authorizable, Modifiable, IAccountingJob { +contract AccountingJob is Authorizable, Modifiable, Job, IAccountingJob { using Encoding for bytes; + using Assertions for address; // --- Data --- @@ -46,7 +47,7 @@ contract AccountingJob is Job, Authorizable, Modifiable, IAccountingJob { address _accountingEngine, address _stabilityFeeTreasury, uint256 _rewardAmount - ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) { + ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) validParams { accountingEngine = IAccountingEngine(_accountingEngine); shouldWorkPopDebtFromQueue = true; @@ -99,17 +100,20 @@ contract AccountingJob is Job, Authorizable, Modifiable, IAccountingJob { // --- Administration --- /// @inheritdoc Modifiable - function _modifyParameters(bytes32 _param, bytes memory _data) internal override { - address _address = _data.toAddress(); + function _modifyParameters(bytes32 _param, bytes memory _data) internal override(Modifiable, Job) { bool _bool = _data.toBool(); - if (_param == 'accountingEngine') accountingEngine = IAccountingEngine(_address); - else if (_param == 'stabilityFeeTreasury') stabilityFeeTreasury = IStabilityFeeTreasury(_address); + if (_param == 'accountingEngine') accountingEngine = IAccountingEngine(_data.toAddress()); else if (_param == 'shouldWorkPopDebtFromQueue') shouldWorkPopDebtFromQueue = _bool; else if (_param == 'shouldWorkAuctionDebt') shouldWorkAuctionDebt = _bool; else if (_param == 'shouldWorkAuctionSurplus') shouldWorkAuctionSurplus = _bool; else if (_param == 'shouldWorkTransferExtraSurplus') shouldWorkTransferExtraSurplus = _bool; - else if (_param == 'rewardAmount') rewardAmount = _data.toUint256(); - else revert UnrecognizedParam(); + else Job._modifyParameters(_param, _data); + } + + /// @inheritdoc Modifiable + function _validateParameters() internal view override(Modifiable, Job) { + address(accountingEngine).assertHasCode(); + Job._validateParameters(); } } diff --git a/src/contracts/jobs/Job.sol b/src/contracts/jobs/Job.sol index 7c4025c1..b303e176 100644 --- a/src/contracts/jobs/Job.sol +++ b/src/contracts/jobs/Job.sol @@ -4,11 +4,21 @@ pragma solidity 0.8.19; import {IJob} from '@interfaces/jobs/IJob.sol'; import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; +import {Authorizable} from '@contracts/utils/Authorizable.sol'; +import {Modifiable} from '@contracts/utils/Modifiable.sol'; + +import {Encoding} from '@libraries/Encoding.sol'; +import {Assertions} from '@libraries/Assertions.sol'; + /** * @title Job Abstract Contract * @notice This abstract contract is inherited by all jobs to add a reward modifier */ -abstract contract Job is IJob { +abstract contract Job is Authorizable, Modifiable, IJob { + using Encoding for bytes; + using Assertions for uint256; + using Assertions for address; + // --- Data --- /// @inheritdoc IJob @@ -31,6 +41,21 @@ abstract contract Job is IJob { rewardAmount = _rewardAmount; } + // --- Administration --- + + /// @inheritdoc Modifiable + function _modifyParameters(bytes32 _param, bytes memory _data) internal virtual override { + if (_param == 'stabilityFeeTreasury') stabilityFeeTreasury = IStabilityFeeTreasury(_data.toAddress()); + else if (_param == 'rewardAmount') rewardAmount = _data.toUint256(); + else revert UnrecognizedParam(); + } + + /// @inheritdoc Modifiable + function _validateParameters() internal view virtual override { + address(stabilityFeeTreasury).assertHasCode(); + rewardAmount.assertNonNull(); + } + // --- Reward --- /// @notice Modifier to reward the caller for calling the function diff --git a/src/contracts/jobs/LiquidationJob.sol b/src/contracts/jobs/LiquidationJob.sol index 4343ffed..336ab378 100644 --- a/src/contracts/jobs/LiquidationJob.sol +++ b/src/contracts/jobs/LiquidationJob.sol @@ -3,7 +3,6 @@ pragma solidity 0.8.19; import {ILiquidationJob} from '@interfaces/jobs/ILiquidationJob.sol'; import {ILiquidationEngine} from '@interfaces/ILiquidationEngine.sol'; -import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; import {Job} from '@contracts/jobs/Job.sol'; @@ -11,13 +10,15 @@ import {Authorizable} from '@contracts/utils/Authorizable.sol'; import {Modifiable} from '@contracts/utils/Modifiable.sol'; import {Encoding} from '@libraries/Encoding.sol'; +import {Assertions} from '@libraries/Assertions.sol'; /** * @title LiquidationJob * @notice This contract contains rewarded methods to handle the SAFE liquidations */ -contract LiquidationJob is Job, Authorizable, Modifiable, ILiquidationJob { +contract LiquidationJob is Authorizable, Modifiable, Job, ILiquidationJob { using Encoding for bytes; + using Assertions for address; // --- Data --- @@ -40,7 +41,7 @@ contract LiquidationJob is Job, Authorizable, Modifiable, ILiquidationJob { address _liquidationEngine, address _stabilityFeeTreasury, uint256 _rewardAmount - ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) { + ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) validParams { liquidationEngine = ILiquidationEngine(_liquidationEngine); shouldWork = true; @@ -67,13 +68,15 @@ contract LiquidationJob is Job, Authorizable, Modifiable, ILiquidationJob { // --- Administration --- /// @inheritdoc Modifiable - function _modifyParameters(bytes32 _param, bytes memory _data) internal override { - address _address = _data.toAddress(); - - if (_param == 'liquidationEngine') liquidationEngine = ILiquidationEngine(_address); - else if (_param == 'stabilityFeeTreasury') stabilityFeeTreasury = IStabilityFeeTreasury(_address); + function _modifyParameters(bytes32 _param, bytes memory _data) internal override(Job, Modifiable) { + if (_param == 'liquidationEngine') liquidationEngine = ILiquidationEngine(_data.toAddress()); else if (_param == 'shouldWork') shouldWork = _data.toBool(); - else if (_param == 'rewardAmount') rewardAmount = _data.toUint256(); - else revert UnrecognizedParam(); + else Job._modifyParameters(_param, _data); + } + + /// @inheritdoc Modifiable + function _validateParameters() internal view override(Job, Modifiable) { + address(liquidationEngine).assertHasCode(); + Job._validateParameters(); } } diff --git a/src/contracts/jobs/OracleJob.sol b/src/contracts/jobs/OracleJob.sol index 9e6af4b2..b913e0b0 100644 --- a/src/contracts/jobs/OracleJob.sol +++ b/src/contracts/jobs/OracleJob.sol @@ -5,7 +5,6 @@ import {IOracleJob} from '@interfaces/jobs/IOracleJob.sol'; import {IOracleRelayer} from '@interfaces/IOracleRelayer.sol'; import {IDelayedOracle} from '@interfaces/oracles/IDelayedOracle.sol'; import {IPIDRateSetter} from '@interfaces/IPIDRateSetter.sol'; -import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; import {Job} from '@contracts/jobs/Job.sol'; @@ -13,13 +12,15 @@ import {Authorizable} from '@contracts/utils/Authorizable.sol'; import {Modifiable} from '@contracts/utils/Modifiable.sol'; import {Encoding} from '@libraries/Encoding.sol'; +import {Assertions} from '@libraries/Assertions.sol'; /** * @title OracleJob * @notice This contract contains rewarded methods to handle the oracle relayer and the PID rate setter updates */ -contract OracleJob is Job, Authorizable, Modifiable, IOracleJob { +contract OracleJob is Authorizable, Modifiable, Job, IOracleJob { using Encoding for bytes; + using Assertions for address; // --- Data --- @@ -48,7 +49,7 @@ contract OracleJob is Job, Authorizable, Modifiable, IOracleJob { address _pidRateSetter, address _stabilityFeeTreasury, uint256 _rewardAmount - ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) { + ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) validParams { oracleRelayer = IOracleRelayer(_oracleRelayer); pidRateSetter = IPIDRateSetter(_pidRateSetter); @@ -96,16 +97,21 @@ contract OracleJob is Job, Authorizable, Modifiable, IOracleJob { // --- Administration --- /// @inheritdoc Modifiable - function _modifyParameters(bytes32 _param, bytes memory _data) internal override { + function _modifyParameters(bytes32 _param, bytes memory _data) internal override(Job, Modifiable) { address _address = _data.toAddress(); bool _bool = _data.toBool(); if (_param == 'oracleRelayer') oracleRelayer = IOracleRelayer(_address); else if (_param == 'pidRateSetter') pidRateSetter = IPIDRateSetter(_address); - else if (_param == 'stabilityFeeTreasury') stabilityFeeTreasury = IStabilityFeeTreasury(_address); else if (_param == 'shouldWorkUpdateCollateralPrice') shouldWorkUpdateCollateralPrice = _bool; else if (_param == 'shouldWorkUpdateRate') shouldWorkUpdateRate = _bool; - else if (_param == 'rewardAmount') rewardAmount = _data.toUint256(); - else revert UnrecognizedParam(); + else Job._modifyParameters(_param, _data); + } + + /// @inheritdoc Modifiable + function _validateParameters() internal view override(Job, Modifiable) { + address(oracleRelayer).assertHasCode(); + address(pidRateSetter).assertHasCode(); + Job._validateParameters(); } } diff --git a/src/interfaces/jobs/IAccountingJob.sol b/src/interfaces/jobs/IAccountingJob.sol index b95dc5af..8ba699d2 100644 --- a/src/interfaces/jobs/IAccountingJob.sol +++ b/src/interfaces/jobs/IAccountingJob.sol @@ -3,12 +3,12 @@ pragma solidity 0.8.19; import {IAccountingEngine} from '@interfaces/IAccountingEngine.sol'; -import {IJob, IStabilityFeeTreasury} from '@interfaces/jobs/IJob.sol'; +import {IJob} from '@interfaces/jobs/IJob.sol'; import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; -interface IAccountingJob is IJob, IAuthorizable, IModifiable { +interface IAccountingJob is IAuthorizable, IModifiable, IJob { // --- Data --- /// @notice Whether the pop debt from queue job should be worked diff --git a/src/interfaces/jobs/IJob.sol b/src/interfaces/jobs/IJob.sol index 8823c5db..389c9439 100644 --- a/src/interfaces/jobs/IJob.sol +++ b/src/interfaces/jobs/IJob.sol @@ -3,7 +3,10 @@ pragma solidity 0.8.19; import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; -interface IJob { +import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; +import {IModifiable} from '@interfaces/utils/IModifiable.sol'; + +interface IJob is IAuthorizable, IModifiable { // --- Events --- /** diff --git a/src/interfaces/jobs/ILiquidationJob.sol b/src/interfaces/jobs/ILiquidationJob.sol index b6674c34..89529070 100644 --- a/src/interfaces/jobs/ILiquidationJob.sol +++ b/src/interfaces/jobs/ILiquidationJob.sol @@ -8,7 +8,7 @@ import {IJob} from '@interfaces/jobs/IJob.sol'; import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; -interface ILiquidationJob is IJob, IAuthorizable, IModifiable { +interface ILiquidationJob is IAuthorizable, IModifiable, IJob { // --- Data --- /// @notice Whether the liquidation job should be worked diff --git a/src/interfaces/jobs/IOracleJob.sol b/src/interfaces/jobs/IOracleJob.sol index 22f3c375..0236de59 100644 --- a/src/interfaces/jobs/IOracleJob.sol +++ b/src/interfaces/jobs/IOracleJob.sol @@ -5,12 +5,12 @@ import {IOracleRelayer} from '@interfaces/IOracleRelayer.sol'; import {IDelayedOracle} from '@interfaces/oracles/IDelayedOracle.sol'; import {IPIDRateSetter} from '@interfaces/IPIDRateSetter.sol'; -import {IJob, IStabilityFeeTreasury} from '@interfaces/jobs/IJob.sol'; +import {IJob} from '@interfaces/jobs/IJob.sol'; import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; -interface IOracleJob is IJob, IAuthorizable, IModifiable { +interface IOracleJob is IAuthorizable, IModifiable, IJob { // --- Errors --- /// @notice Throws when trying to update an invalid collateral price diff --git a/test/testnet/mocks/JobForTest.sol b/test/testnet/mocks/JobForTest.sol index 5deb84e5..ed432b09 100644 --- a/test/testnet/mocks/JobForTest.sol +++ b/test/testnet/mocks/JobForTest.sol @@ -2,9 +2,13 @@ pragma solidity 0.8.19; import {Job, IJob} from '@contracts/jobs/Job.sol'; +import {Authorizable} from '@contracts/utils/Authorizable.sol'; contract JobForTest is Job { - constructor(address _stabilityFeeTreasury, uint256 _rewardAmount) Job(_stabilityFeeTreasury, _rewardAmount) {} + constructor( + address _stabilityFeeTreasury, + uint256 _rewardAmount + ) Job(_stabilityFeeTreasury, _rewardAmount) Authorizable(msg.sender) validParams {} function rewardModifier() external reward {} } diff --git a/test/testnet/unit/jobs/AccountingJob.t.sol b/test/testnet/unit/jobs/AccountingJob.t.sol index 9ea2c140..090adc3d 100644 --- a/test/testnet/unit/jobs/AccountingJob.t.sol +++ b/test/testnet/unit/jobs/AccountingJob.t.sol @@ -9,6 +9,8 @@ import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; import {HaiTest, stdStorage, StdStorage} from '@testnet/utils/HaiTest.t.sol'; +import {Assertions} from '@libraries/Assertions.sol'; + abstract contract Base is HaiTest { using stdStorage for StdStorage; @@ -76,6 +78,13 @@ contract Unit_AccountingJob_Constructor is Base { _; } + function test_Emit_AddAuthorization() public happyPath { + vm.expectEmit(); + emit AddAuthorization(user); + + new AccountingJobForTest(address(mockAccountingEngine), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + function test_Set_StabilityFeeTreasury() public happyPath { assertEq(address(accountingJob.stabilityFeeTreasury()), address(mockStabilityFeeTreasury)); } @@ -84,15 +93,7 @@ contract Unit_AccountingJob_Constructor is Base { assertEq(accountingJob.rewardAmount(), REWARD_AMOUNT); } - function test_Emit_AddAuthorization() public happyPath { - vm.expectEmit(); - emit AddAuthorization(user); - - accountingJob = - new AccountingJobForTest(address(mockAccountingEngine), address(mockStabilityFeeTreasury), REWARD_AMOUNT); - } - - function test_Set_AccountingEngine(address _accountingEngine) public happyPath { + function test_Set_AccountingEngine(address _accountingEngine) public happyPath mockAsContract(_accountingEngine) { accountingJob = new AccountingJobForTest(_accountingEngine, address(mockStabilityFeeTreasury), REWARD_AMOUNT); assertEq(address(accountingJob.accountingEngine()), _accountingEngine); @@ -113,6 +114,24 @@ contract Unit_AccountingJob_Constructor is Base { function test_Set_ShouldWorkTransferExtraSurplus() public happyPath { assertEq(accountingJob.shouldWorkTransferExtraSurplus(), true); } + + function test_Revert_Null_AccountingEngine() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new AccountingJobForTest(address(0), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new AccountingJobForTest(address(mockAccountingEngine), address(0), REWARD_AMOUNT); + } + + function test_Revert_Null_RewardAmount() public { + vm.expectRevert(Assertions.NullAmount.selector); + + new AccountingJobForTest(address(mockAccountingEngine), address(mockStabilityFeeTreasury), 0); + } } contract Unit_AccountingJob_WorkPopDebtFromQueue is Base { @@ -261,20 +280,22 @@ contract Unit_AccountingJob_WorkTransferExtraSurplus is Base { } contract Unit_AccountingJob_ModifyParameters is Base { - event ModifyParameters(bytes32 indexed _param, bytes32 indexed _cType, bytes _data); - modifier happyPath() { vm.startPrank(authorizedAccount); _; } - function test_Set_AccountingEngine(address _accountingEngine) public happyPath { + function test_Set_AccountingEngine(address _accountingEngine) public happyPath mockAsContract(_accountingEngine) { accountingJob.modifyParameters('accountingEngine', abi.encode(_accountingEngine)); assertEq(address(accountingJob.accountingEngine()), _accountingEngine); } - function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath { + function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) + public + happyPath + mockAsContract(_stabilityFeeTreasury) + { accountingJob.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(accountingJob.stabilityFeeTreasury()), _stabilityFeeTreasury); @@ -305,11 +326,37 @@ contract Unit_AccountingJob_ModifyParameters is Base { } function test_Set_RewardAmount(uint256 _rewardAmount) public happyPath { + vm.assume(_rewardAmount != 0); + accountingJob.modifyParameters('rewardAmount', abi.encode(_rewardAmount)); assertEq(accountingJob.rewardAmount(), _rewardAmount); } + function test_Revert_Null_AccountingEngine() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + accountingJob.modifyParameters('accountingEngine', abi.encode(address(0))); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + accountingJob.modifyParameters('stabilityFeeTreasury', abi.encode(address(0))); + } + + function test_Revert_Null_RewardAmount() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(Assertions.NullAmount.selector); + + accountingJob.modifyParameters('rewardAmount', abi.encode(0)); + } + function test_Revert_UnrecognizedParam(bytes memory _data) public { vm.startPrank(authorizedAccount); diff --git a/test/testnet/unit/jobs/Job.t.sol b/test/testnet/unit/jobs/Job.t.sol index ff53fced..998125a1 100644 --- a/test/testnet/unit/jobs/Job.t.sol +++ b/test/testnet/unit/jobs/Job.t.sol @@ -3,12 +3,17 @@ pragma solidity 0.8.19; import {JobForTest, IJob} from '@testnet/mocks/JobForTest.sol'; import {IStabilityFeeTreasury} from '@interfaces/IStabilityFeeTreasury.sol'; +import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; +import {IModifiable} from '@interfaces/utils/IModifiable.sol'; import {HaiTest, stdStorage, StdStorage} from '@testnet/utils/HaiTest.t.sol'; +import {Assertions} from '@libraries/Assertions.sol'; + abstract contract Base is HaiTest { using stdStorage for StdStorage; address deployer = label('deployer'); + address authorizedAccount = label('authorizedAccount'); address user = label('user'); IStabilityFeeTreasury mockStabilityFeeTreasury = IStabilityFeeTreasury(mockContract('StabilityFeeTreasury')); @@ -18,9 +23,14 @@ abstract contract Base is HaiTest { uint256 constant REWARD_AMOUNT = 1e18; function setUp() public virtual { - vm.prank(deployer); + vm.startPrank(deployer); + job = new JobForTest(address(mockStabilityFeeTreasury), REWARD_AMOUNT); label(address(job), 'Job'); + + job.addAuthorization(authorizedAccount); + + vm.stopPrank(); } function _mockRewardAmount(uint256 _rewardAmount) internal { @@ -29,17 +39,88 @@ abstract contract Base is HaiTest { } contract Unit_Job_Constructor is Base { - function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public { - job = new JobForTest(_stabilityFeeTreasury, REWARD_AMOUNT); + event AddAuthorization(address _account); + + modifier happyPath() { + vm.startPrank(user); + _; + } + + function test_Emit_AddAuthorization() public happyPath { + vm.expectEmit(); + emit AddAuthorization(user); + + new JobForTest(address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + + function test_Set_StabilityFeeTreasury() public happyPath { + assertEq(address(job.stabilityFeeTreasury()), address(mockStabilityFeeTreasury)); + } + + function test_Set_RewardAmount() public happyPath { + assertEq(job.rewardAmount(), REWARD_AMOUNT); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new JobForTest(address(0), REWARD_AMOUNT); + } + + function test_Revert_Null_RewardAmount() public { + vm.expectRevert(Assertions.NullAmount.selector); + + new JobForTest(address(mockStabilityFeeTreasury), 0); + } +} + +contract Unit_Job_ModifyParameters is Base { + modifier happyPath() { + vm.startPrank(authorizedAccount); + _; + } + + function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) + public + happyPath + mockAsContract(_stabilityFeeTreasury) + { + job.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(job.stabilityFeeTreasury()), _stabilityFeeTreasury); } - function test_Set_RewardAmount(uint256 _rewardAmount) public { - job = new JobForTest(address(mockStabilityFeeTreasury), _rewardAmount); + function test_Set_RewardAmount(uint256 _rewardAmount) public happyPath { + vm.assume(_rewardAmount != 0); + + job.modifyParameters('rewardAmount', abi.encode(_rewardAmount)); assertEq(job.rewardAmount(), _rewardAmount); } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + job.modifyParameters('stabilityFeeTreasury', abi.encode(address(0))); + } + + function test_Revert_Null_RewardAmount() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(Assertions.NullAmount.selector); + + job.modifyParameters('rewardAmount', abi.encode(0)); + } + + function test_Revert_UnrecognizedParam(bytes memory _data) public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(IModifiable.UnrecognizedParam.selector); + + job.modifyParameters('unrecognizedParam', _data); + } } contract Unit_Job_Reward is Base { diff --git a/test/testnet/unit/jobs/LiquidationJob.t.sol b/test/testnet/unit/jobs/LiquidationJob.t.sol index 1526c12c..92de71d9 100644 --- a/test/testnet/unit/jobs/LiquidationJob.t.sol +++ b/test/testnet/unit/jobs/LiquidationJob.t.sol @@ -9,6 +9,8 @@ import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; import {HaiTest, stdStorage, StdStorage} from '@testnet/utils/HaiTest.t.sol'; +import {Assertions} from '@libraries/Assertions.sol'; + abstract contract Base is HaiTest { using stdStorage for StdStorage; @@ -61,6 +63,13 @@ contract Unit_LiquidationJob_Constructor is Base { _; } + function test_Emit_AddAuthorization() public happyPath { + vm.expectEmit(); + emit AddAuthorization(user); + + new LiquidationJobForTest(address(mockLiquidationEngine), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + function test_Set_StabilityFeeTreasury() public happyPath { assertEq(address(liquidationJob.stabilityFeeTreasury()), address(mockStabilityFeeTreasury)); } @@ -69,15 +78,7 @@ contract Unit_LiquidationJob_Constructor is Base { assertEq(liquidationJob.rewardAmount(), REWARD_AMOUNT); } - function test_Emit_AddAuthorization() public happyPath { - vm.expectEmit(); - emit AddAuthorization(user); - - liquidationJob = - new LiquidationJobForTest(address(mockLiquidationEngine), address(mockStabilityFeeTreasury), REWARD_AMOUNT); - } - - function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { + function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath mockAsContract(_liquidationEngine) { liquidationJob = new LiquidationJobForTest(_liquidationEngine, address(mockStabilityFeeTreasury), REWARD_AMOUNT); assertEq(address(liquidationJob.liquidationEngine()), _liquidationEngine); @@ -86,6 +87,24 @@ contract Unit_LiquidationJob_Constructor is Base { function test_Set_ShouldWork() public happyPath { assertEq(liquidationJob.shouldWork(), true); } + + function test_Revert_Null_LiquidationEngine() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new LiquidationJobForTest(address(0), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new LiquidationJobForTest(address(mockLiquidationEngine), address(0), REWARD_AMOUNT); + } + + function test_Revert_Null_RewardAmount() public { + vm.expectRevert(Assertions.NullAmount.selector); + + new LiquidationJobForTest(address(mockLiquidationEngine), address(mockStabilityFeeTreasury), 0); + } } contract Unit_LiquidationJob_WorkLiquidation is Base { @@ -133,8 +152,6 @@ contract Unit_LiquidationJob_WorkLiquidation is Base { } contract Unit_LiquidationJob_ModifyParameters is Base { - event ModifyParameters(bytes32 indexed _param, bytes32 indexed _cType, bytes _data); - modifier happyPath() { vm.startPrank(authorizedAccount); _; @@ -159,11 +176,37 @@ contract Unit_LiquidationJob_ModifyParameters is Base { } function test_Set_RewardAmount(uint256 _rewardAmount) public happyPath { + vm.assume(_rewardAmount != 0); + liquidationJob.modifyParameters('rewardAmount', abi.encode(_rewardAmount)); assertEq(liquidationJob.rewardAmount(), _rewardAmount); } + function test_Revert_Null_LiquidationEngine() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + liquidationJob.modifyParameters('liquidationEngine', abi.encode(address(0))); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + liquidationJob.modifyParameters('stabilityFeeTreasury', abi.encode(address(0))); + } + + function test_Revert_Null_RewardAmount() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(Assertions.NullAmount.selector); + + liquidationJob.modifyParameters('rewardAmount', abi.encode(0)); + } + function test_Revert_UnrecognizedParam(bytes memory _data) public { vm.startPrank(authorizedAccount); diff --git a/test/testnet/unit/jobs/OracleJob.t.sol b/test/testnet/unit/jobs/OracleJob.t.sol index 60b97ea5..f8c136e3 100644 --- a/test/testnet/unit/jobs/OracleJob.t.sol +++ b/test/testnet/unit/jobs/OracleJob.t.sol @@ -11,6 +11,8 @@ import {IAuthorizable} from '@interfaces/utils/IAuthorizable.sol'; import {IModifiable} from '@interfaces/utils/IModifiable.sol'; import {HaiTest, stdStorage, StdStorage} from '@testnet/utils/HaiTest.t.sol'; +import {Assertions} from '@libraries/Assertions.sol'; + abstract contract Base is HaiTest { using stdStorage for StdStorage; @@ -80,6 +82,13 @@ contract Unit_OracleJob_Constructor is Base { _; } + function test_Emit_AddAuthorization() public happyPath { + vm.expectEmit(); + emit AddAuthorization(user); + + new OracleJobForTest(address(mockOracleRelayer), address(mockPIDRateSetter), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + function test_Set_StabilityFeeTreasury() public happyPath { assertEq(address(oracleJob.stabilityFeeTreasury()), address(mockStabilityFeeTreasury)); } @@ -88,23 +97,14 @@ contract Unit_OracleJob_Constructor is Base { assertEq(oracleJob.rewardAmount(), REWARD_AMOUNT); } - function test_Emit_AddAuthorization() public happyPath { - vm.expectEmit(); - emit AddAuthorization(user); - - oracleJob = new OracleJobForTest( - address(mockOracleRelayer), address(mockPIDRateSetter), address(mockStabilityFeeTreasury), REWARD_AMOUNT - ); - } - - function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { + function test_Set_OracleRelayer(address _oracleRelayer) public happyPath mockAsContract(_oracleRelayer) { oracleJob = new OracleJobForTest(_oracleRelayer, address(mockPIDRateSetter), address(mockStabilityFeeTreasury), REWARD_AMOUNT); assertEq(address(oracleJob.oracleRelayer()), _oracleRelayer); } - function test_Set_PIDRateSetter(address _pidRateSetter) public happyPath { + function test_Set_PIDRateSetter(address _pidRateSetter) public happyPath mockAsContract(_pidRateSetter) { oracleJob = new OracleJobForTest(address(mockOracleRelayer), _pidRateSetter, address(mockStabilityFeeTreasury), REWARD_AMOUNT); @@ -118,6 +118,30 @@ contract Unit_OracleJob_Constructor is Base { function test_Set_ShouldWorkUpdateRate() public happyPath { assertEq(oracleJob.shouldWorkUpdateRate(), true); } + + function test_Revert_Null_OracleRelayer() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new OracleJobForTest(address(0), address(mockPIDRateSetter), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + + function test_Revert_Null_PIDRateSetter() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new OracleJobForTest(address(mockOracleRelayer), address(0), address(mockStabilityFeeTreasury), REWARD_AMOUNT); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + new OracleJobForTest(address(mockOracleRelayer), address(mockPIDRateSetter), address(0), REWARD_AMOUNT); + } + + function test_Revert_Null_RewardAmount() public { + vm.expectRevert(Assertions.NullAmount.selector); + + new OracleJobForTest(address(mockOracleRelayer), address(mockPIDRateSetter), address(mockStabilityFeeTreasury), 0); + } } contract Unit_OracleJob_WorkUpdateCollateralPrice is Base { @@ -215,26 +239,28 @@ contract Unit_OracleJob_WorkUpdateRate is Base { } contract Unit_OracleJob_ModifyParameters is Base { - event ModifyParameters(bytes32 indexed _param, bytes32 indexed _cType, bytes _data); - modifier happyPath() { vm.startPrank(authorizedAccount); _; } - function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { + function test_Set_OracleRelayer(address _oracleRelayer) public happyPath mockAsContract(_oracleRelayer) { oracleJob.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); assertEq(address(oracleJob.oracleRelayer()), _oracleRelayer); } - function test_Set_PIDRateSetter(address _pidRateSetter) public happyPath { + function test_Set_PIDRateSetter(address _pidRateSetter) public happyPath mockAsContract(_pidRateSetter) { oracleJob.modifyParameters('pidRateSetter', abi.encode(_pidRateSetter)); assertEq(address(oracleJob.pidRateSetter()), _pidRateSetter); } - function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath { + function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) + public + happyPath + mockAsContract(_stabilityFeeTreasury) + { oracleJob.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(oracleJob.stabilityFeeTreasury()), _stabilityFeeTreasury); @@ -253,11 +279,45 @@ contract Unit_OracleJob_ModifyParameters is Base { } function test_Set_RewardAmount(uint256 _rewardAmount) public happyPath { + vm.assume(_rewardAmount != 0); + oracleJob.modifyParameters('rewardAmount', abi.encode(_rewardAmount)); assertEq(oracleJob.rewardAmount(), _rewardAmount); } + function test_Revert_Null_OracleRelayer() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + oracleJob.modifyParameters('oracleRelayer', abi.encode(address(0))); + } + + function test_Revert_Null_PIDRateSetter() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + oracleJob.modifyParameters('pidRateSetter', abi.encode(address(0))); + } + + function test_Revert_Null_StabilityFeeTreasury() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(abi.encodeWithSelector(Assertions.NoCode.selector, address(0))); + + oracleJob.modifyParameters('stabilityFeeTreasury', abi.encode(address(0))); + } + + function test_Revert_Null_RewardAmount() public { + vm.startPrank(authorizedAccount); + + vm.expectRevert(Assertions.NullAmount.selector); + + oracleJob.modifyParameters('rewardAmount', abi.encode(0)); + } + function test_Revert_UnrecognizedParam(bytes memory _data) public { vm.startPrank(authorizedAccount); From 114b470d68d663e6a20536ada96af0beefe9b36a Mon Sep 17 00:00:00 2001 From: fabiohild Date: Thu, 8 Feb 2024 22:50:10 -0300 Subject: [PATCH 3/4] fix tests --- .gitmodules | 9 ++--- lib/forge-std | 2 +- test/testnet/unit/AccountingEngine.t.sol | 6 +-- .../testnet/unit/CollateralAuctionHouse.t.sol | 10 ++--- test/testnet/unit/DebtAuctionHouse.t.sol | 6 +-- test/testnet/unit/GlobalSettlement.t.sol | 39 +++++++------------ test/testnet/unit/LiquidationEngine.t.sol | 24 ++++-------- test/testnet/unit/OracleRelayer.t.sol | 4 +- test/testnet/unit/PIDRateSetter.t.sol | 6 +-- .../PostSettlementSurplusAuctionHouse.t.sol | 6 +-- test/testnet/unit/SurplusAuctionHouse.t.sol | 6 +-- test/testnet/unit/jobs/LiquidationJob.t.sol | 4 +- test/testnet/utils/HaiTest.t.sol | 22 ++++++++++- 13 files changed, 62 insertions(+), 82 deletions(-) diff --git a/.gitmodules b/.gitmodules index c1149790..dc65e37b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,14 +1,13 @@ [submodule "lib/ds-test"] path = lib/ds-test url = https://github.com/dapphub/ds-test -[submodule "lib/forge-std"] - path = lib/forge-std - url = https://github.com/brockelmore/forge-std - branch = chore/v1.5.4 [submodule "lib/isolmate"] path = lib/isolmate url = https://github.com/defi-wonderland/isolmate [submodule "lib/prb-test"] path = lib/prb-test url = https://github.com/paulrberg/prb-test - branch = 0.5.5 \ No newline at end of file + branch = 0.5.5 +[submodule "lib/forge-std"] + path = lib/forge-std + url = https://github.com/foundry-rs/forge-std diff --git a/lib/forge-std b/lib/forge-std index 6c5971b8..ae570fec 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 6c5971b8e8d7ff85985bf1b716a455c511e9962b +Subproject commit ae570fec082bfe1c1f45b0acca4a2b4f84d345ce diff --git a/test/testnet/unit/AccountingEngine.t.sol b/test/testnet/unit/AccountingEngine.t.sol index ee550785..661550bb 100644 --- a/test/testnet/unit/AccountingEngine.t.sol +++ b/test/testnet/unit/AccountingEngine.t.sol @@ -250,9 +250,8 @@ contract Unit_AccountingEngine_ModifyParameters is Base { assertEq(abi.encode(_fuzz), abi.encode(_params)); } - function test_ModifyParameters_SurplusAuctionHouse(address _surplusAuctionHouse) public authorized { + function test_ModifyParameters_SurplusAuctionHouse(address _surplusAuctionHouse) public authorized mockAsContract(_surplusAuctionHouse) { vm.assume(_surplusAuctionHouse != address(0)); - vm.etch(_surplusAuctionHouse, '0xF'); address _previousSurplusAuctionHouse = address(accountingEngine.surplusAuctionHouse()); if (_previousSurplusAuctionHouse != address(0)) { vm.expectCall( @@ -270,9 +269,8 @@ contract Unit_AccountingEngine_ModifyParameters is Base { assertEq(_surplusAuctionHouse, address(accountingEngine.surplusAuctionHouse())); } - function test_ModifyParameters_DebtAuctionHouse(address _debtAuctionHouse) public authorized { + function test_ModifyParameters_DebtAuctionHouse(address _debtAuctionHouse) public authorized mockAsContract(_debtAuctionHouse) { vm.assume(_debtAuctionHouse != address(0)); - vm.etch(_debtAuctionHouse, '0xF'); accountingEngine.modifyParameters('debtAuctionHouse', abi.encode(_debtAuctionHouse)); assertEq(_debtAuctionHouse, address(accountingEngine.debtAuctionHouse())); diff --git a/test/testnet/unit/CollateralAuctionHouse.t.sol b/test/testnet/unit/CollateralAuctionHouse.t.sol index f903d859..4224ff5b 100644 --- a/test/testnet/unit/CollateralAuctionHouse.t.sol +++ b/test/testnet/unit/CollateralAuctionHouse.t.sol @@ -1606,13 +1606,11 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { assertEq(abi.encode(_params), abi.encode(_fuzz)); } - function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { + function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath mockAsContract(_liquidationEngine) { vm.assume(_liquidationEngine != address(0)); vm.assume(_liquidationEngine != deployer); vm.assume(_liquidationEngine != authorizedAccount); - vm.etch(_liquidationEngine, '0xF'); - collateralAuctionHouse.modifyParameters('liquidationEngine', abi.encode(_liquidationEngine)); assertEq(address(collateralAuctionHouse.liquidationEngine()), _liquidationEngine); @@ -1621,8 +1619,7 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { function test_Emit_Authorization_LiquidationEngine( address _oldLiquidationEngine, address _newLiquidationEngine - ) public happyPath { - vm.etch(_newLiquidationEngine, '0xF'); + ) public happyPath mockAsContract(_newLiquidationEngine) { vm.assume(_newLiquidationEngine != address(0)); vm.assume(_newLiquidationEngine != deployer); vm.assume(_newLiquidationEngine != authorizedAccount); @@ -1643,9 +1640,8 @@ contract Unit_CollateralAuctionHouse_ModifyParameters is Base { collateralAuctionHouse.modifyParameters('liquidationEngine', abi.encode(_newLiquidationEngine)); } - function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { + function test_Set_OracleRelayer(address _oracleRelayer) public happyPath mockAsContract(_oracleRelayer) { vm.assume(_oracleRelayer != address(0)); - vm.etch(_oracleRelayer, '0xF'); collateralAuctionHouse.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); diff --git a/test/testnet/unit/DebtAuctionHouse.t.sol b/test/testnet/unit/DebtAuctionHouse.t.sol index 0d670fa1..17369327 100644 --- a/test/testnet/unit/DebtAuctionHouse.t.sol +++ b/test/testnet/unit/DebtAuctionHouse.t.sol @@ -157,9 +157,8 @@ contract Unit_DebtAuctionHouse_Constructor is Base { assertEq(address(debtAuctionHouse.safeEngine()), _safeEngine); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); debtAuctionHouse = new DebtAuctionHouseForTest(address(mockSafeEngine), _protocolToken, dahParams); assertEq(address(debtAuctionHouse.protocolToken()), _protocolToken); @@ -930,9 +929,8 @@ contract Unit_DebtAuctionHouse_ModifyParameters is Base { assertEq(abi.encode(_params), abi.encode(_fuzz)); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); debtAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(debtAuctionHouse.protocolToken()), _protocolToken); diff --git a/test/testnet/unit/GlobalSettlement.t.sol b/test/testnet/unit/GlobalSettlement.t.sol index bd21cd41..4e5cf468 100644 --- a/test/testnet/unit/GlobalSettlement.t.sol +++ b/test/testnet/unit/GlobalSettlement.t.sol @@ -242,9 +242,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.safeEngine()), _safeEngine); } - function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { + function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath mockAsContract(_liquidationEngine) { vm.assume(address(_liquidationEngine) != address(0)); - vm.etch(_liquidationEngine, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(_liquidationEngine), @@ -259,9 +258,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.liquidationEngine()), _liquidationEngine); } - function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { + function test_Set_OracleRelayer(address _oracleRelayer) public happyPath mockAsContract(_oracleRelayer) { vm.assume(address(_oracleRelayer) != address(0)); - vm.etch(_oracleRelayer, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -276,9 +274,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.oracleRelayer()), _oracleRelayer); } - function test_Set_CoinJoin(address _coinJoin) public happyPath { + function test_Set_CoinJoin(address _coinJoin) public happyPath mockAsContract(_coinJoin) { vm.assume(address(_coinJoin) != address(0)); - vm.etch(_coinJoin, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -293,9 +290,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.coinJoin()), _coinJoin); } - function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath { + function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath mockAsContract(_collateralJoinFactory) { vm.assume(address(_collateralJoinFactory) != address(0)); - vm.etch(_collateralJoinFactory, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -310,9 +306,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.collateralJoinFactory()), _collateralJoinFactory); } - function test_Set_CollateralAuctionHouseFactory(address _collateralAuctionHouseFactory) public happyPath { + function test_Set_CollateralAuctionHouseFactory(address _collateralAuctionHouseFactory) public happyPath mockAsContract(_collateralAuctionHouseFactory) { vm.assume(address(_collateralAuctionHouseFactory) != address(0)); - vm.etch(_collateralAuctionHouseFactory, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -327,9 +322,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.collateralAuctionHouseFactory()), _collateralAuctionHouseFactory); } - function test_Set_StabilityFeeTreasury(address _sfTreasury) public happyPath { + function test_Set_StabilityFeeTreasury(address _sfTreasury) public happyPath mockAsContract(_sfTreasury) { vm.assume(address(_sfTreasury) != address(0)); - vm.etch(_sfTreasury, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -344,9 +338,8 @@ contract Unit_GlobalSettlement_Constructor is Base { assertEq(address(globalSettlement.stabilityFeeTreasury()), _sfTreasury); } - function test_Set_AccountingEngine(address _accountingEngine) public happyPath { + function test_Set_AccountingEngine(address _accountingEngine) public happyPath mockAsContract(_accountingEngine) { vm.assume(address(_accountingEngine) != address(0)); - vm.etch(_accountingEngine, '0xF'); globalSettlement = new GlobalSettlement( address(mockSafeEngine), address(mockLiquidationEngine), @@ -1287,49 +1280,43 @@ contract Unit_GlobalSettlement_ModifyParameters is Base { globalSettlement.modifyParameters(_param, _data); } - function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { + function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath mockAsContract(_liquidationEngine) { vm.assume(_liquidationEngine != address(0)); - vm.etch(_liquidationEngine, '0xF'); globalSettlement.modifyParameters('liquidationEngine', abi.encode(_liquidationEngine)); assertEq(address(globalSettlement.liquidationEngine()), _liquidationEngine); } - function test_Set_AccountingEngine(address _accountingEngine) public happyPath { + function test_Set_AccountingEngine(address _accountingEngine) public happyPath mockAsContract(_accountingEngine) { vm.assume(_accountingEngine != address(0)); - vm.etch(_accountingEngine, '0xF'); globalSettlement.modifyParameters('accountingEngine', abi.encode(_accountingEngine)); assertEq(address(globalSettlement.accountingEngine()), _accountingEngine); } - function test_Set_OracleRelayer(address _oracleRelayer) public happyPath { + function test_Set_OracleRelayer(address _oracleRelayer) public happyPath mockAsContract(_oracleRelayer) { vm.assume(_oracleRelayer != address(0)); - vm.etch(_oracleRelayer, '0xF'); globalSettlement.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); assertEq(address(globalSettlement.oracleRelayer()), _oracleRelayer); } - function test_Set_CoinJoin(address _coinJoin) public happyPath { + function test_Set_CoinJoin(address _coinJoin) public happyPath mockAsContract(_coinJoin) { vm.assume(_coinJoin != address(0)); - vm.etch(_coinJoin, '0xF'); globalSettlement.modifyParameters('coinJoin', abi.encode(_coinJoin)); assertEq(address(globalSettlement.coinJoin()), _coinJoin); } - function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath { + function test_Set_CollateralJoinFactory(address _collateralJoinFactory) public happyPath mockAsContract(_collateralJoinFactory){ vm.assume(_collateralJoinFactory != address(0)); - vm.etch(_collateralJoinFactory, '0xF'); globalSettlement.modifyParameters('collateralJoinFactory', abi.encode(_collateralJoinFactory)); assertEq(address(globalSettlement.collateralJoinFactory()), _collateralJoinFactory); } - function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath { + function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath mockAsContract(_stabilityFeeTreasury) { vm.assume(_stabilityFeeTreasury != address(0)); - vm.etch(_stabilityFeeTreasury, '0xF'); globalSettlement.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(globalSettlement.stabilityFeeTreasury()), _stabilityFeeTreasury); diff --git a/test/testnet/unit/LiquidationEngine.t.sol b/test/testnet/unit/LiquidationEngine.t.sol index 3c24ac36..8160733d 100644 --- a/test/testnet/unit/LiquidationEngine.t.sol +++ b/test/testnet/unit/LiquidationEngine.t.sol @@ -333,11 +333,9 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { function test_ModifyParameters_PerCollateral( bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _fuzz - ) public authorized { + ) public authorized mockAsContract(_fuzz.collateralAuctionHouse) { _mockCollateralList(_cType); - vm.etch(_fuzz.collateralAuctionHouse, '0xF'); - vm.assume(_fuzz.collateralAuctionHouse != address(0)); vm.assume(_fuzz.collateralAuctionHouse != deployer); liquidationEngine.modifyParameters(_cType, 'collateralAuctionHouse', abi.encode(_fuzz.collateralAuctionHouse)); @@ -364,9 +362,8 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { liquidationEngine.modifyParameters(_cType, 'liquidationQuantity', abi.encode(_liquidationQuantity)); } - function test_ModifyParameters_AccountingEngine(address _accountingEngine) public authorized { + function test_ModifyParameters_AccountingEngine(address _accountingEngine) public authorized mockAsContract(_accountingEngine) { vm.assume(_accountingEngine != address(0)); - vm.etch(_accountingEngine, '0xF'); liquidationEngine.modifyParameters('accountingEngine', abi.encode(_accountingEngine)); assertEq(_accountingEngine, address(liquidationEngine.accountingEngine())); @@ -376,15 +373,13 @@ contract Unit_LiquidationEngine_ModifyParameters is Base { bytes32 _cType, address _previousCAH, address _newCAH - ) public authorized { + ) public authorized mockAsContract(_newCAH) { _mockCollateralList(_cType); vm.assume(_newCAH != address(0)); vm.assume(_newCAH != deployer); vm.assume(_previousCAH != deployer); - vm.etch(_newCAH, '0xF'); - LiquidationEngineForTest(address(liquidationEngine)).setCollateralAuctionHouse(_cType, _previousCAH); if (_previousCAH != address(0)) { @@ -1805,8 +1800,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { function test_Set_CParams( bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams - ) public authorized happyPath(_liqEngineCParams) { - vm.etch(address(_liqEngineCParams.collateralAuctionHouse), '0xF'); + ) public authorized happyPath(_liqEngineCParams) mockAsContract(_liqEngineCParams.collateralAuctionHouse) { liquidationEngine.initializeCollateralType(_cType, _liqEngineCParams); assertEq(abi.encode(liquidationEngine.cParams(_cType)), abi.encode(_liqEngineCParams)); @@ -1815,8 +1809,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { function test_Call_SAFEEngine_ApproveSAFEModification( bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams - ) public authorized happyPath(_liqEngineCParams) { - vm.etch(_liqEngineCParams.collateralAuctionHouse, '0xF'); + ) public authorized happyPath(_liqEngineCParams) mockAsContract(_liqEngineCParams.collateralAuctionHouse) { vm.expectCall( address(mockSafeEngine), abi.encodeCall(mockSafeEngine.approveSAFEModification, (_liqEngineCParams.collateralAuctionHouse)) @@ -1828,8 +1821,7 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { function test_Emit_AddAuthorization( bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams - ) public authorized happyPath(_liqEngineCParams) { - vm.etch(_liqEngineCParams.collateralAuctionHouse, '0xF'); + ) public authorized happyPath(_liqEngineCParams) mockAsContract(_liqEngineCParams.collateralAuctionHouse) { vm.expectEmit(); emit AddAuthorization(_liqEngineCParams.collateralAuctionHouse); @@ -1850,13 +1842,11 @@ contract Unit_LiquidationEngine_InitializeCollateralType is Base { function test_Revert_LiquidationQuantity_NotLesserOrEqualThan( bytes32 _cType, ILiquidationEngine.LiquidationEngineCollateralParams memory _liqEngineCParams - ) public authorized { + ) public authorized mockAsContract(_liqEngineCParams.collateralAuctionHouse) { vm.assume(_liqEngineCParams.collateralAuctionHouse != address(0)); vm.assume(_liqEngineCParams.collateralAuctionHouse != deployer); vm.assume(_liqEngineCParams.liquidationQuantity > MAX_RAD); - vm.etch(address(_liqEngineCParams.collateralAuctionHouse), '0xF'); - vm.expectRevert( abi.encodeWithSelector(Assertions.NotLesserOrEqualThan.selector, _liqEngineCParams.liquidationQuantity, MAX_RAD) ); diff --git a/test/testnet/unit/OracleRelayer.t.sol b/test/testnet/unit/OracleRelayer.t.sol index 03e1d33c..384acc9d 100644 --- a/test/testnet/unit/OracleRelayer.t.sol +++ b/test/testnet/unit/OracleRelayer.t.sol @@ -200,16 +200,14 @@ contract Unit_OracleRelayer_ModifyParameters is Base { bytes32 _cType, IOracleRelayer.OracleRelayerCollateralParams memory _fuzz, address _fuzzPriceSource - ) public authorized previousValidCTypeParams(_cType) { + ) public authorized previousValidCTypeParams(_cType) mockAsContract(address(_fuzz.oracle)) { vm.assume(_validOracleRelayerCollateralParams(_fuzz)); // NOTE: needs to have a valid liqCRatio to pass the `modifyParameters` check _mockCTypeLiquidationCRatio(_cType, 1e27); - vm.etch(_fuzzPriceSource, '0xF'); vm.mockCall( address(_fuzz.oracle), abi.encodeWithSelector(IDelayedOracle.priceSource.selector), abi.encode(_fuzzPriceSource) ); - vm.etch(address(_fuzz.oracle), '0xF'); oracleRelayer.modifyParameters(_cType, 'oracle', abi.encode(_fuzz.oracle)); oracleRelayer.modifyParameters(_cType, 'safetyCRatio', abi.encode(_fuzz.safetyCRatio)); diff --git a/test/testnet/unit/PIDRateSetter.t.sol b/test/testnet/unit/PIDRateSetter.t.sol index c9e0c0e1..fbf57206 100644 --- a/test/testnet/unit/PIDRateSetter.t.sol +++ b/test/testnet/unit/PIDRateSetter.t.sol @@ -121,17 +121,15 @@ contract Unit_PIDRateSetter_ModifyParameters is Base { assertEq(abi.encode(_fuzz), abi.encode(_params)); } - function test_ModifyParameters_Set_OracleRelayer(address _oracleRelayer) public authorized { + function test_ModifyParameters_Set_OracleRelayer(address _oracleRelayer) public authorized mockAsContract(_oracleRelayer) { vm.assume(_oracleRelayer != address(0)); - vm.etch(_oracleRelayer, '0xF'); pidRateSetter.modifyParameters('oracleRelayer', abi.encode(_oracleRelayer)); assertEq(address(pidRateSetter.oracleRelayer()), _oracleRelayer); } - function test_ModifyParameters_Set_PIDCalculator(address _pidCalculator) public authorized { + function test_ModifyParameters_Set_PIDCalculator(address _pidCalculator) public authorized mockAsContract(_pidCalculator) { vm.assume(_pidCalculator != address(0)); - vm.etch(_pidCalculator, '0xF'); pidRateSetter.modifyParameters('pidCalculator', abi.encode(_pidCalculator)); assertEq(address(pidRateSetter.pidCalculator()), _pidCalculator); diff --git a/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol b/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol index 6c01bc2e..81d54193 100644 --- a/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol +++ b/test/testnet/unit/PostSettlementSurplusAuctionHouse.t.sol @@ -114,9 +114,8 @@ contract Unit_PostSettlementSurplusAuctionHouse_Constructor is Base { assertEq(address(postSettlementSurplusAuctionHouse.safeEngine()), _safeEngine); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); postSettlementSurplusAuctionHouse = new PostSettlementSurplusAuctionHouseForTest(address(mockSafeEngine), _protocolToken, pssahParams); @@ -680,9 +679,8 @@ contract Unit_PostSettlementSurplusAuctionHouse_ModifyParameters is Base { assertEq(abi.encode(_params), abi.encode(_fuzz)); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); postSettlementSurplusAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(postSettlementSurplusAuctionHouse.protocolToken()), _protocolToken); diff --git a/test/testnet/unit/SurplusAuctionHouse.t.sol b/test/testnet/unit/SurplusAuctionHouse.t.sol index d4b81abe..f14e0413 100644 --- a/test/testnet/unit/SurplusAuctionHouse.t.sol +++ b/test/testnet/unit/SurplusAuctionHouse.t.sol @@ -147,9 +147,8 @@ contract Unit_SurplusAuctionHouse_Constructor is Base { assertEq(address(surplusAuctionHouse.safeEngine()), _safeEngine); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); surplusAuctionHouse = new SurplusAuctionHouseForTest(address(mockSafeEngine), _protocolToken, sahParams); assertEq(address(surplusAuctionHouse.protocolToken()), _protocolToken); @@ -916,9 +915,8 @@ contract Unit_SurplusAuctionHouse_ModifyParameters is Base { assertEq(abi.encode(_params), abi.encode(_fuzz)); } - function test_Set_ProtocolToken(address _protocolToken) public happyPath { + function test_Set_ProtocolToken(address _protocolToken) public happyPath mockAsContract(_protocolToken) { vm.assume(_protocolToken != address(0)); - vm.etch(_protocolToken, '0xF'); surplusAuctionHouse.modifyParameters('protocolToken', abi.encode(_protocolToken)); assertEq(address(surplusAuctionHouse.protocolToken()), _protocolToken); diff --git a/test/testnet/unit/jobs/LiquidationJob.t.sol b/test/testnet/unit/jobs/LiquidationJob.t.sol index 92de71d9..29f79afd 100644 --- a/test/testnet/unit/jobs/LiquidationJob.t.sol +++ b/test/testnet/unit/jobs/LiquidationJob.t.sol @@ -157,13 +157,13 @@ contract Unit_LiquidationJob_ModifyParameters is Base { _; } - function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath { + function test_Set_LiquidationEngine(address _liquidationEngine) public happyPath mockAsContract(_liquidationEngine) { liquidationJob.modifyParameters('liquidationEngine', abi.encode(_liquidationEngine)); assertEq(address(liquidationJob.liquidationEngine()), _liquidationEngine); } - function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath { + function test_Set_StabilityFeeTreasury(address _stabilityFeeTreasury) public happyPath mockAsContract(_stabilityFeeTreasury) { liquidationJob.modifyParameters('stabilityFeeTreasury', abi.encode(_stabilityFeeTreasury)); assertEq(address(liquidationJob.stabilityFeeTreasury()), _stabilityFeeTreasury); diff --git a/test/testnet/utils/HaiTest.t.sol b/test/testnet/utils/HaiTest.t.sol index fd52cdf1..d2e6cea4 100644 --- a/test/testnet/utils/HaiTest.t.sol +++ b/test/testnet/utils/HaiTest.t.sol @@ -131,4 +131,24 @@ contract OverflowChecker { } } -abstract contract HaiTest is DSTestPlus, OverflowChecker {} +abstract contract HaiTest is DSTestPlus, OverflowChecker { + modifier mockAsContract(address _address) { + // Foundry fuzzer sometimes gives us the next deployment address + // this results in very unexpected reverts as any contract deploy will revert + // we check here to make sure it's not the next deployment address for the (pranked) msg.sender + (, address _msgSender,) = vm.readCallers(); + address _nextDeploymentAddr = computeCreateAddress(address(_msgSender), vm.getNonce(_msgSender)); + + vm.assume(_address != _nextDeploymentAddr); + + // It should not be a precompile + vm.assume(uint160(_address) > 20); + + // It should not be a deployed contract + vm.assume(_address.code.length == 0); + + // Give it bytecode to make it a contract + vm.etch(_address, '0xF'); + _; + } +} From 040443e39ef793ee288ffd71cb7915c429445150 Mon Sep 17 00:00:00 2001 From: fabiohild Date: Thu, 8 Feb 2024 23:16:12 -0300 Subject: [PATCH 4/4] preventing calls to address(0) on ODProxy --- src/contracts/proxies/ODProxy.sol | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/contracts/proxies/ODProxy.sol b/src/contracts/proxies/ODProxy.sol index d3e16c2c..2392eeff 100644 --- a/src/contracts/proxies/ODProxy.sol +++ b/src/contracts/proxies/ODProxy.sol @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity 0.8.19; +import {Address} from '@openzeppelin/utils/Address.sol'; // Open Dollar // Version 1.6.1 contract ODProxy { + using Address for address; error TargetAddressRequired(); error TargetCallFailed(bytes _response); error OnlyOwner(); @@ -26,11 +28,6 @@ contract ODProxy { function execute(address _target, bytes memory _data) external payable onlyOwner returns (bytes memory _response) { if (_target == address(0)) revert TargetAddressRequired(); - bool _succeeded; - (_succeeded, _response) = _target.delegatecall(_data); - - if (!_succeeded) { - revert TargetCallFailed(_response); - } + _response = _target.functionDelegateCall(_data); } }