LiquidStone audit remediation fixes (#158)

LiquidStone audit remediation fixes --------- Co-authored-by: Ian Lucas <[email protected]> Co-authored-by: Jonathan Lodge <[email protected]> Co-authored-by: Nawar Hisso <[email protected]> Co-authored-by: Chai Somsri <[email protected]>
credbull · Oct 29, 2024 · 5581781 · 5581781
1 parent 930e288
commit 5581781
Show file tree

Hide file tree

Showing 19 changed files with 1,325 additions and 216 deletions.
diff --git a/packages/contracts/package.json b/packages/contracts/package.json
@@ -10,7 +10,7 @@
     "dev": "yarn rm-dbdata && anvil --config-out localhost.json & make deploy-local",
     "build": "forge build && yarn gen-types",
     "test": "forge test",
-    "coverage": "forge coverage --report lcov && genhtml lcov.info -o out/test-reports/coverage --ignore-errors inconsistent",
+    "coverage": "forge coverage --report lcov && genhtml lcov.info --branch-coverage -o out/test-reports/coverage --ignore-errors inconsistent",
     "format": "forge fmt && prettier './script/**/*.js' --write",
     "lint": "forge fmt && eslint --fix --ignore-path .gitignore && yarn solhint './*(test|src)/**/*.sol'",
     "db-check": "tsc && node ./script/utils/checkDb.js",

diff --git a/packages/contracts/src/timelock/ITimelockAsyncUnlock.sol b/packages/contracts/src/timelock/ITimelockAsyncUnlock.sol
@@ -31,6 +31,13 @@ interface ITimelockAsyncUnlock {
         external
         returns (uint256[] memory depositPeriods, uint256[] memory amounts);
 
+    /**
+     * @notice Cancel a pending request to unlock
+     * @param owner Owner of the request
+     * @param requestId Discriminator between non-fungible requests
+     */
+    function cancelRequestUnlock(address owner, uint256 requestId) external;
+
     /**
      * @dev Return notice period
      */

diff --git a/packages/contracts/src/timelock/TimelockAsyncUnlock.sol b/packages/contracts/src/timelock/TimelockAsyncUnlock.sol
@@ -22,6 +22,8 @@ abstract contract TimelockAsyncUnlock is Initializable, ITimelockAsyncUnlock, Co
     // cache of user requested unlocks by depositPeriod across ALL requests.  maps account => map(depositPeriod -> unlockAmount)
     mapping(address account => EnumerableMap.UintToUintMap) private _depositPeriodAmountCache;
 
+    event CancelRedeemRequest(address indexed owner, uint256 indexed requestId, address indexed sender);
+
     error TimelockAsyncUnlock__AuthorizeCallerFailed(address caller, address owner);
     error TimelockAsyncUnlock__InvalidArrayLength(uint256 depositPeriodsLength, uint256 amountsLength);
     error TimelockAsyncUnlock__ExceededMaxRequestUnlock(
@@ -33,7 +35,7 @@ abstract contract TimelockAsyncUnlock is Initializable, ITimelockAsyncUnlock, Co
     error TimelockAsyncUnlock__UnlockBeforeDepositPeriod(
         address caller, address owner, uint256 depositPeriod, uint256 unlockPeriod
     );
-    error TimelockAsyncUnlock__UnlockBeforeUnlockPeriod(
+    error TimelockAsyncUnlock__UnlockBeforeCurrentPeriod(
         address caller, address owner, uint256 currentPeriod, uint256 unlockPeriod
     );
 
@@ -181,6 +183,10 @@ abstract contract TimelockAsyncUnlock is Initializable, ITimelockAsyncUnlock, Co
     {
         _authorizeCaller(_msgSender(), owner);
 
+        if (requestId > currentPeriod()) {
+            revert TimelockAsyncUnlock__UnlockBeforeCurrentPeriod(_msgSender(), owner, currentPeriod(), requestId);
+        }
+
         // use copy of the depositPeriods and amounts.  we will be altering the storage in _unlock()
         (depositPeriods, amounts) = unlockRequests(owner, requestId);
 
@@ -200,7 +206,9 @@ abstract contract TimelockAsyncUnlock is Initializable, ITimelockAsyncUnlock, Co
         internal
         virtual
     {
-        _handleUnlockValidation(owner, depositPeriod, requestId);
+        if (requestId < depositPeriod) {
+            revert TimelockAsyncUnlock__UnlockBeforeDepositPeriod(_msgSender(), owner, depositPeriod, requestId);
+        }
 
         EnumerableMap.UintToUintMap storage unlockRequestsForRequestId = _unlockRequests[owner][requestId];
 
@@ -269,19 +277,6 @@ abstract contract TimelockAsyncUnlock is Initializable, ITimelockAsyncUnlock, Co
         }
     }
 
-    /**
-     * @dev An internal function to check if unlock can be performed
-     */
-    function _handleUnlockValidation(address owner, uint256 depositPeriod, uint256 unlockPeriod) internal virtual {
-        if (unlockPeriod > currentPeriod()) {
-            revert TimelockAsyncUnlock__UnlockBeforeUnlockPeriod(_msgSender(), owner, currentPeriod(), unlockPeriod);
-        }
-
-        if (unlockPeriod < depositPeriod) {
-            revert TimelockAsyncUnlock__UnlockBeforeDepositPeriod(_msgSender(), owner, depositPeriod, unlockPeriod);
-        }
-    }
-
     function supportsInterface(bytes4 interfaceId) public view virtual returns (bool) {
         return interfaceId == type(ITimelockAsyncUnlock).interfaceId;
     }

diff --git a/packages/contracts/src/yield/CalcInterestMetadata.sol b/packages/contracts/src/yield/CalcInterestMetadata.sol
@@ -40,4 +40,11 @@ abstract contract CalcInterestMetadata is Initializable, ICalcInterestMetadata {
     function scale() public view virtual returns (uint256 scale_) {
         return SCALE;
     }
+
+    /**
+     * @dev This empty reserved space is put in place to allow future versions to add new
+     * variables without shifting down storage in the inheritance chain.
+     * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
+     */
+    uint256[49] private __gap;
 }
diff --git a/packages/contracts/src/yield/LiquidContinuousMultiTokenVault.sol b/packages/contracts/src/yield/LiquidContinuousMultiTokenVault.sol
@@ -73,7 +73,8 @@ contract LiquidContinuousMultiTokenVault is
 
     error LiquidContinuousMultiTokenVault__InvalidFrequency(uint256 frequency);
     error LiquidContinuousMultiTokenVault__InvalidAuthAddress(string authName, address authAddress);
-    error LiquidContinuousMultiTokenVault__ControllerMismatch(address sender, address controller);
+    error LiquidContinuousMultiTokenVault__ControllerNotSender(address sender, address controller);
+    error LiquidContinuousMultiTokenVault__UnAuthorized(address sender, address authorizedOwner);
     error LiquidContinuousMultiTokenVault__AmountMismatch(uint256 amount1, uint256 amount2);
     error LiquidContinuousMultiTokenVault__UnlockPeriodMismatch(uint256 unlockPeriod1, uint256 unlockPeriod2);
     error LiquidContinuousMultiTokenVault__InvalidComponentTokenAmount(
@@ -125,7 +126,7 @@ contract LiquidContinuousMultiTokenVault is
         override
         returns (uint256 shares)
     {
-        if (assets < SCALE) return 0; // no shares for fractional principal
+        if (assets < _minConversionThreshold()) return 0; // no shares for small fractional assets
 
         return assets; // 1 asset = 1 share
     }
@@ -161,7 +162,7 @@ contract LiquidContinuousMultiTokenVault is
         override
         returns (uint256 assets)
     {
-        if (shares < SCALE) return 0; // no assets for fractional shares
+        if (shares < _minConversionThreshold()) return 0; // no assets for small fractional shares
 
         if (redeemPeriod < depositPeriod) return 0; // trying to redeem before depositPeriod
 
@@ -179,11 +180,12 @@ contract LiquidContinuousMultiTokenVault is
      * @param owner Source of the assets to deposit
      * @return requestId_ Discriminator between non-fungible requests
      */
-    function requestDeposit(uint256 assets, address controller, address owner) public returns (uint256 requestId_) {
-        if (controller != _msgSender()) {
-            revert LiquidContinuousMultiTokenVault__ControllerMismatch(_msgSender(), controller);
-        }
-
+    function requestDeposit(uint256 assets, address controller, address owner)
+        public
+        onlyAuthorized(owner)
+        onlyController(controller)
+        returns (uint256 requestId_)
+    {
         uint256 requestId = ZERO_REQUEST_ID; // requests and requestIds not used in buys.
 
         deposit(assets, owner, controller);
@@ -196,11 +198,11 @@ contract LiquidContinuousMultiTokenVault is
      * @param assets Amount of `asset` that was deposited by `requestDeposit`
      * @param receiver Address to receive the shares
      */
-    function deposit(uint256 assets, address receiver, address controller) public returns (uint256 shares_) {
-        if (controller != _msgSender()) {
-            revert LiquidContinuousMultiTokenVault__ControllerMismatch(_msgSender(), controller);
-        }
-
+    function deposit(uint256 assets, address receiver, address controller)
+        public
+        onlyController(controller)
+        returns (uint256 shares_)
+    {
         uint256 shares = deposit(assets, receiver);
         emit Deposit(controller, receiver, assets, shares);
         return shares;
@@ -212,42 +214,49 @@ contract LiquidContinuousMultiTokenVault is
      * @param owner Source of the shares to redeem
      * @return requestId_ Discriminator between non-fungible requests
      */
-    function requestRedeem(uint256 shares, address, /* controller */ address owner)
+    function requestRedeem(uint256 shares, address controller, address owner)
         public
+        onlyAuthorized(owner)
+        onlyController(controller)
         returns (uint256 requestId_)
     {
         // using optimize() variant in case "shares" represents the IComponent "principal + yield" which is our "assets".
         (uint256[] memory depositPeriods, uint256[] memory sharesAtPeriods) =
             _redeemOptimizer.optimize(this, owner, shares, shares, minUnlockPeriod());
 
-        uint256 requestId = requestUnlock(_msgSender(), depositPeriods, sharesAtPeriods);
-        emit RedeemRequest(_msgSender(), owner, requestId, _msgSender(), shares);
+        uint256 requestId = requestUnlock(owner, depositPeriods, sharesAtPeriods);
+        emit RedeemRequest(controller, owner, requestId, _msgSender(), shares);
         return requestId;
     }
 
     /**
      * @notice Fulfill a request to redeem assets by transferring assets to the receiver
      * @param shares Amount of shares that was redeemed by `requestRedeem`
      * @param receiver Address to receive the assets
+     * @dev controller will only have tokens to redeem if they are also the owner
      */
-    function redeem(uint256 shares, address receiver, address /* controller */ ) public returns (uint256 assets) {
+    function redeem(uint256 shares, address receiver, address controller)
+        public
+        onlyController(controller)
+        returns (uint256 assets)
+    {
         uint256 requestId = currentPeriod(); // requestId = redeemPeriod, and redeem can only be called  where redeemPeriod = currentPeriod()
 
-        uint256 unlockRequestedAmount = unlockRequestAmount(receiver, requestId);
+        uint256 unlockRequestedAmount = unlockRequestAmount(controller, requestId);
         if (shares != unlockRequestedAmount) {
             revert LiquidContinuousMultiTokenVault__InvalidComponentTokenAmount(shares, unlockRequestedAmount);
         }
 
-        (uint256[] memory depositPeriods, uint256[] memory sharesAtPeriods) = unlock(receiver, requestId); // unlockPeriod = redeemPeriod
+        (uint256[] memory depositPeriods, uint256[] memory sharesAtPeriods) = unlock(controller, requestId); // unlockPeriod = redeemPeriod
 
         uint256 totalAssetsRedeemed = 0;
 
         for (uint256 i = 0; i < depositPeriods.length; ++i) {
             totalAssetsRedeemed += _redeemForDepositPeriodAfterUnlock(
-                sharesAtPeriods[i], receiver, _msgSender(), depositPeriods[i], requestId
+                sharesAtPeriods[i], receiver, controller, depositPeriods[i], requestId
             );
         }
-        emit Withdraw(_msgSender(), receiver, _msgSender(), totalAssetsRedeemed, shares);
+        emit Withdraw(_msgSender(), receiver, controller, totalAssetsRedeemed, shares);
         return totalAssetsRedeemed;
     }
 
@@ -367,6 +376,12 @@ contract LiquidContinuousMultiTokenVault is
 
     /// @dev yield based on the associated yieldStrategy
     function calcYield(uint256 principal, uint256 fromPeriod, uint256 toPeriod) public view returns (uint256 yield) {
+        // no yield earned when depositing and requesting redeem within the notice period.
+        // e.g. deposit day 1, immediately request redeem on day 1.  should give 0 returns.
+        if (toPeriod <= fromPeriod + noticePeriod()) {
+            return 0;
+        }
+
         return _yieldStrategy.calcYield(address(this), principal, fromPeriod, toPeriod);
     }
 
@@ -383,6 +398,17 @@ contract LiquidContinuousMultiTokenVault is
         _depositForDepositPeriod(amount, account, depositPeriod);
     }
 
+    /// @dev Cancel a pending request to unlock
+    function cancelRequestUnlock(address owner, uint256 requestId) public onlyAuthorized(owner) {
+        (uint256[] memory depositPeriods, uint256[] memory amounts) = unlockRequests(owner, requestId);
+
+        for (uint256 i = 0; i < depositPeriods.length; ++i) {
+            _unlock(owner, depositPeriods[i], requestId, amounts[i]);
+        }
+
+        emit CancelRedeemRequest(owner, requestId, _msgSender());
+    }
+
     /// @inheritdoc TimelockAsyncUnlock
     function lockedAmount(address account, uint256 depositPeriod)
         public
@@ -393,6 +419,13 @@ contract LiquidContinuousMultiTokenVault is
         return balanceOf(account, depositPeriod);
     }
 
+    /// @inheritdoc TimelockAsyncUnlock
+    function _authorizeCaller(address caller, address owner) internal virtual override {
+        if (caller != owner && !isApprovedForAll(owner, caller)) {
+            revert LiquidContinuousMultiTokenVault__UnAuthorized(caller, owner);
+        }
+    }
+
     // ===================== TripleRateContext =====================
 
     /// @inheritdoc TripleRateContext
@@ -458,6 +491,27 @@ contract LiquidContinuousMultiTokenVault is
 
     // ===================== Utility =====================
 
+    /// minimum shares required to convert to assets and vice-versa.
+    function _minConversionThreshold() internal view returns (uint256 minConversionThreshold) {
+        return SCALE < 10 ? SCALE : 10;
+    }
+
+    // @dev ensure caller is permitted to act on the owner's tokens
+    modifier onlyAuthorized(address owner) {
+        _authorizeCaller(_msgSender(), owner);
+        _;
+    }
+
+    // @dev ensure the controller is the caller
+    modifier onlyController(address controller) {
+        address caller = _msgSender();
+
+        if (caller != controller) {
+            revert LiquidContinuousMultiTokenVault__ControllerNotSender(caller, controller);
+        }
+        _;
+    }
+
     function pause() public onlyRole(OPERATOR_ROLE) {
         _pause();
     }

diff --git a/packages/contracts/src/yield/strategy/TripleRateYieldStrategy.sol b/packages/contracts/src/yield/strategy/TripleRateYieldStrategy.sol
@@ -102,7 +102,7 @@ contract TripleRateYieldStrategy is AbstractYieldStrategy {
         ITripleRateContext context = ITripleRateContext(contextContract);
 
         return CalcSimpleInterest.calcPriceFromInterest(
-            numPeriodsElapsed, context.rateScaled(), context.frequency(), context.scale()
+            context.rateScaled(), numPeriodsElapsed, context.frequency(), context.scale()
         );
     }
 

diff --git a/packages/contracts/test/src/timelock/TimelockAsyncUnlockTest.t.sol b/packages/contracts/test/src/timelock/TimelockAsyncUnlockTest.t.sol
@@ -185,7 +185,7 @@ contract TimelockAsyncUnlockTest is Test {
 
         vm.expectRevert(
             abi.encodeWithSelector(
-                TimelockAsyncUnlock.TimelockAsyncUnlock__UnlockBeforeUnlockPeriod.selector,
+                TimelockAsyncUnlock.TimelockAsyncUnlock__UnlockBeforeCurrentPeriod.selector,
                 alice,
                 alice,
                 asyncUnlock.currentPeriod(),