Merge pull request #16 from lidofinance/feature/second_round_audit_re…

…view

Second round audit fixes

contracts/lib/UnstructuredStorage.sol

@@ -10,14 +10,6 @@ library UnstructuredStorage {
         assembly { data := sload(position) }
     }
 
-    function getStorageAddress(bytes32 position) internal view returns (address data) {
-        assembly { data := sload(position) }
-    }
-
-    function getStorageBytes32(bytes32 position) internal view returns (bytes32 data) {
-        assembly { data := sload(position) }
-    }
-
     function getStorageUint256(bytes32 position) internal view returns (uint256 data) {
         assembly { data := sload(position) }
     }
@@ -26,14 +18,6 @@ library UnstructuredStorage {
         assembly { sstore(position, data) }
     }
 
-    function setStorageAddress(bytes32 position, address data) internal {
-        assembly { sstore(position, data) }
-    }
-
-    function setStorageBytes32(bytes32 position, bytes32 data) internal {
-        assembly { sstore(position, data) }
-    }
-
     function setStorageUint256(bytes32 position, uint256 data) internal {
         assembly { sstore(position, data) }
     }

contracts/lido/TokenRateNotifier.sol

@@ -27,6 +27,9 @@ interface IPostTokenRebaseReceiver {
 contract TokenRateNotifier is Ownable, IPostTokenRebaseReceiver {
     using ERC165Checker for address;
 
+    /// @notice Address of lido core protocol contract that is allowed to call handlePostTokenRebase.
+    address public immutable LIDO;
+
     /// @notice Maximum amount of observers to be supported.
     uint256 public constant MAX_OBSERVERS_COUNT = 32;
 
@@ -40,11 +43,16 @@ contract TokenRateNotifier is Ownable, IPostTokenRebaseReceiver {
     address[] public observers;
 
     /// @param initialOwner_ initial owner
-    constructor(address initialOwner_) {
+    /// @param lido_ Address of lido core protocol contract that is allowed to call handlePostTokenRebase.
+    constructor(address initialOwner_, address lido_) {
         if (initialOwner_ == address(0)) {
             revert ErrorZeroAddressOwner();
         }
+        if (lido_ == address(0)) {
+            revert ErrorZeroAddressLido();
+        }
         _transferOwnership(initialOwner_);
+        LIDO = lido_;
     }
 
     /// @notice Add a `observer_` to the back of array
@@ -85,6 +93,7 @@ contract TokenRateNotifier is Ownable, IPostTokenRebaseReceiver {
 
     /// @inheritdoc IPostTokenRebaseReceiver
     /// @dev Parameters aren't used because all required data further components fetch by themselves.
+    ///      Allowed to called by Lido contract. See Lido._completeTokenRebase.
     function handlePostTokenRebase(
         uint256, /* reportTimestamp    */
         uint256, /* timeElapsed        */
@@ -94,6 +103,9 @@ contract TokenRateNotifier is Ownable, IPostTokenRebaseReceiver {
         uint256, /* postTotalEther     */
         uint256  /* sharesMintedAsFees */
     ) external {
+        if (msg.sender != LIDO) {
+            revert ErrorNotAuthorizedRebaseCaller();
+        }
         uint256 cachedObserversLength = observers.length;
         for (uint256 obIndex = 0; obIndex < cachedObserversLength; obIndex++) {
             // solhint-disable-next-line no-empty-blocks
@@ -141,5 +153,7 @@ contract TokenRateNotifier is Ownable, IPostTokenRebaseReceiver {
     error ErrorMaxObserversCountExceeded();
     error ErrorNoObserverToRemove();
     error ErrorZeroAddressOwner();
+    error ErrorZeroAddressLido();
+    error ErrorNotAuthorizedRebaseCaller();
     error ErrorAddExistedObserver();
 }

contracts/optimism/TokenRateOracle.sol

@@ -55,8 +55,8 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
     ///         See the 'pauseTokenRateUpdates()' method
     uint256 public immutable OLDEST_RATE_ALLOWED_IN_PAUSE_TIME_SPAN;
 
-    /// @notice The maximum delta time that is allowed between two L1 timestamps of token rate updates.
-    uint256 public immutable MAX_ALLOWED_TIME_BETWEEN_TOKEN_RATE_UPDATES;
+    /// @notice The minimum delta time between two L1 timestamps of token rate updates.
+    uint256 public immutable MIN_TIME_BETWEEN_TOKEN_RATE_UPDATES;
 
     /// @notice Decimals of the oracle response.
     uint8 public constant DECIMALS = 27;
@@ -95,7 +95,7 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
     ///        Can't be bigger than BASIS_POINT_SCALE.
     /// @param oldestRateAllowedInPauseTimeSpan_ Maximum allowed time difference between the current time
     ///        and the last received token rate update that can be set during a pause.
-    /// @param maxAllowedTimeBetweenTokenRateUpdates_ the maximum delta time that is allowed between two
+    /// @param minTimeBetweenTokenRateUpdates_ Minimum delta time between two
     ///        L1 timestamps of token rate updates.
     constructor(
         address messenger_,
@@ -105,7 +105,7 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
         uint256 maxAllowedL2ToL1ClockLag_,
         uint256 maxAllowedTokenRateDeviationPerDayBp_,
         uint256 oldestRateAllowedInPauseTimeSpan_,
-        uint256 maxAllowedTimeBetweenTokenRateUpdates_
+        uint256 minTimeBetweenTokenRateUpdates_
     ) CrossDomainEnabled(messenger_) {
         if (l2ERC20TokenBridge_ == address(0)) {
             revert ErrorZeroAddressL2ERC20TokenBridge();
@@ -122,7 +122,7 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
         MAX_ALLOWED_L2_TO_L1_CLOCK_LAG = maxAllowedL2ToL1ClockLag_;
         MAX_ALLOWED_TOKEN_RATE_DEVIATION_PER_DAY_BP = maxAllowedTokenRateDeviationPerDayBp_;
         OLDEST_RATE_ALLOWED_IN_PAUSE_TIME_SPAN = oldestRateAllowedInPauseTimeSpan_;
-        MAX_ALLOWED_TIME_BETWEEN_TOKEN_RATE_UPDATES = maxAllowedTimeBetweenTokenRateUpdates_;
+        MIN_TIME_BETWEEN_TOKEN_RATE_UPDATES = minTimeBetweenTokenRateUpdates_;
     }
 
     /// @notice Initializes the contract from scratch.
@@ -180,6 +180,9 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
         if (rateUpdatedL1Timestamp_ > block.timestamp + MAX_ALLOWED_L2_TO_L1_CLOCK_LAG) {
             revert ErrorL1TimestampExceededMaxAllowedClockLag(rateUpdatedL1Timestamp_);
         }
+        if (rateUpdatedL1Timestamp_ < _getLastTokenRate().rateUpdatedL1Timestamp) {
+            revert ErrorL1TimestampOlderThanPrevious(rateUpdatedL1Timestamp_);
+        }
         _addTokenRate(tokenRate_, rateUpdatedL1Timestamp_, block.timestamp);
         _setPause(false);
         emit TokenRateUpdatesResumed(tokenRate_, rateUpdatedL1Timestamp_);
@@ -263,8 +266,8 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
             return;
         }
 
-        /// @dev This condition was made under the assumption that the L1 timestamps can be hacked.
-        if (rateUpdatedL1Timestamp_ < tokenRateData.rateUpdatedL1Timestamp + MAX_ALLOWED_TIME_BETWEEN_TOKEN_RATE_UPDATES) {
+        /// @dev This condition was made under the assumption that the L1 timestamps can be manipulated.
+        if (rateUpdatedL1Timestamp_ < tokenRateData.rateUpdatedL1Timestamp + MIN_TIME_BETWEEN_TOKEN_RATE_UPDATES) {
             emit UpdateRateIsTooOften(rateUpdatedL1Timestamp_, tokenRateData.rateUpdatedL1Timestamp);
             return;
         }
@@ -428,4 +431,5 @@ contract TokenRateOracle is ITokenRateOracle, CrossDomainEnabled, AccessControl,
     error ErrorMaxTokenRateDeviationIsOutOfRange();
     error ErrorTokenRateIsOutOfSaneRange(uint256 tokenRate_);
     error ErrorL1TimestampExceededMaxAllowedClockLag(uint256 rateL1Timestamp_);
+    error ErrorL1TimestampOlderThanPrevious(uint256 rateL1Timestamp_);
 }

contracts/token/ERC20RebasableBridged.sol

@@ -415,7 +415,6 @@ contract ERC20RebasableBridged is IERC20, IERC20Wrapper, IBridgeWrapper, ERC20Me
     error ErrorZeroSharesWrap();
     error ErrorZeroTokensUnwrap();
     error ErrorZeroSharesUnwrap();
-    error ErrorTokenRateDecimalsIsZero();
     error ErrorTransferToRebasableContract();
     error ErrorNotEnoughBalance();
     error ErrorNotEnoughAllowance();

package.json

@@ -7,21 +7,19 @@
     "compile": "hardhat compile",
     "compile:force": "hardhat compile --force",
     "coverage": "hardhat coverage --testfiles './test/**/*.unit.test.ts'",
-    "test:e2e": "hardhat test ./test/**/*.e2e.test.ts",
-    "test:unit": "hardhat test ./test/**/*.unit.test.ts",
-    "test:integration": "hardhat test ./test/**/*.integration.test.ts",
     "fork:eth:mainnet": "hardhat node:fork eth_mainnet 8545",
     "fork:eth:sepolia": "hardhat node:fork eth_sepolia 8545",
     "fork:opt:sepolia": "hardhat node:fork opt_sepolia 9545",
     "fork:opt:mainnet": "hardhat node:fork opt_mainnet 9545",
-    "optimism:deploy": "ts-node --files ./scripts/optimism/deploy-bridge.ts",
-    "optimism:finalize-message": "ts-node --files ./scripts/optimism/finalize-message.ts",
-    "optimism:test:e2e": "hardhat test ./test/optimism/*.e2e.test.ts",
-    "optimism:test:unit": "hardhat test ./test/optimism/*.unit.test.ts",
-    "optimism:test:integration": "hardhat test ./test/optimism/*.integration.test.ts",
-    "optimism:test:acceptance": "hardhat test ./test/optimism/*.acceptance.test.ts",
-    "optimism:test:executor": "hardhat test ./test/bridge-executor/optimism.integration.test.ts",
-    "optimism:test:launch": "REVERT=false hardhat test ./test/optimism/{_launch.test.ts,bridging.integration.test.ts}"
+    "deploy": "ts-node --files ./scripts/optimism/deploy-scratch.ts",
+    "upgrade": "ts-node --files ./scripts/optimism/upgrade.ts",
+    "finalize-message": "ts-node --files ./scripts/optimism/finalize-message.ts",
+    "test:e2e": "hardhat test ./test/optimism/*.e2e.test.ts",
+    "test:unit": "hardhat test ./test/optimism/*.unit.test.ts",
+    "test:integration": "hardhat test ./test/optimism/*.integration.test.ts",
+    "test:acceptance": "hardhat test ./test/optimism/*.acceptance.test.ts",
+    "test:executor": "hardhat test ./test/bridge-executor/optimism.integration.test.ts",
+    "test:launch": "REVERT=false hardhat test ./test/optimism/{_launch.test.ts,bridging.integration.test.ts}"
   },
   "keywords": [],
   "author": "",