Merge pull request #485 from open-dollar/feature/hai-audit-fixes

Audit release: Feature/hai audit fixes

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();
   }
 }

src/contracts/CollateralAuctionHouse.sol

@@ -396,8 +396,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);

src/contracts/DebtAuctionHouse.sol

@@ -221,6 +221,6 @@ contract DebtAuctionHouse is Authorizable, Modifiable, Disableable, IDebtAuction
 
   /// @inheritdoc Modifiable
   function _validateParameters() internal view override {
-    address(protocolToken).assertNonNull();
+    address(protocolToken).assertHasCode();
   }
 }

src/contracts/LiquidationEngine.sol

@@ -301,13 +301,13 @@ contract LiquidationEngine is
 
   /// @inheritdoc Modifiable
   function _validateParameters() internal view override {
-    address(accountingEngine).assertNonNull();
+    address(accountingEngine).assertHasCode();
   }
 
   /// @inheritdoc ModifiablePerCollateral
   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);
   }
 

src/contracts/OracleRelayer.sol

@@ -169,7 +169,7 @@ contract OracleRelayer is Authorizable, Disableable, Modifiable, ModifiablePerCo
   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();
@@ -190,22 +190,22 @@ contract OracleRelayer is Authorizable, Disableable, Modifiable, ModifiablePerCo
   /// @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();
   }
 
   /// @inheritdoc Modifiable
   function _validateParameters() internal view override {
     _params.redemptionRateUpperBound.assertGt(RAY);
     _params.redemptionRateLowerBound.assertGt(0).assertLt(RAY);
-    address(systemCoinOracle).assertNonNull();
+    address(systemCoinOracle).assertHasCode();
   }
 
   /// @inheritdoc ModifiablePerCollateral
   function _validateCParameters(bytes32 _cType) internal view override {
     OracleRelayerCollateralParams memory __cParams = _cParams[_cType];
     __cParams.safetyCRatio.assertGtEq(__cParams.liquidationCRatio);
     __cParams.liquidationCRatio.assertGtEq(RAY);
-    address(__cParams.oracle).assertNonNull();
+    address(__cParams.oracle).assertHasCode();
   }
 }

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();
   }
 }

src/contracts/SurplusAuctionHouse.sol

@@ -139,16 +139,30 @@ contract SurplusAuctionHouse is Authorizable, Modifiable, Disableable, ISurplusA
     if (_bid <= _auction.bidAmount) revert SAH_BidNotHigher();
     if (_bid * WAD < _params.bidIncrease * _auction.bidAmount) revert SAH_InsufficientIncrease();
 
-    if (msg.sender != _auction.highBidder) {
-      // If there was no previous bid then no transfer is needed
-      if (_auction.bidAmount != 0) protocolToken.safeTransferFrom(msg.sender, _auction.highBidder, _auction.bidAmount);
+    // The amount that will be transferred to the auction house
+    uint256 _deltaBidAmount = _bid;
+
+    // Check if this is the first bid or not
+    if (_auction.bidExpiry != 0) {
+      // Since this is not the first bid, it might be that we need to repay the previous bidder
+      if (msg.sender != _auction.highBidder) {
+        protocolToken.safeTransferFrom(msg.sender, _auction.highBidder, _auction.bidAmount);
+
+        _auction.highBidder = msg.sender;
+      }
+      // Either we just repaid the previous bidder,
+      // or this user is also the previous bidder and is incrementing his bid
+      _deltaBidAmount -= _auction.bidAmount;
+    } else {
+      // This is the first bid
       _auction.highBidder = msg.sender;
     }
-    protocolToken.safeTransferFrom(msg.sender, address(this), _bid - _auction.bidAmount);
 
     _auction.bidAmount = _bid;
     _auction.bidExpiry = block.timestamp + _params.bidDuration;
 
+    protocolToken.safeTransferFrom(msg.sender, address(this), _deltaBidAmount);
+
     emit IncreaseBidSize({
       _id: _id,
       _bidder: msg.sender,
@@ -222,7 +236,7 @@ contract SurplusAuctionHouse is Authorizable, Modifiable, Disableable, ISurplusA
 
   /// @inheritdoc Modifiable
   function _validateParameters() internal view override {
-    address(protocolToken).assertNonNull();
+    address(protocolToken).assertHasCode();
     _params.bidReceiver.assertNonNull();
   }
 }

src/contracts/jobs/AccountingJob.sol

@@ -3,21 +3,22 @@ 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';
 
 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();
   }
 }

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

src/contracts/jobs/LiquidationJob.sol

@@ -3,21 +3,22 @@ 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';
 
 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();
   }
 }

src/contracts/jobs/OracleJob.sol

@@ -5,21 +5,22 @@ 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';
 
 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();
   }
 }

src/contracts/proxies/ODProxy.sol

@@ -1,10 +1,14 @@
 // 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 +30,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);
   }
 }