M-04 [Oval] Updating Parameters Inside the BaseController Contract Ma…

…y Lead to Undesired Outcomes (#19)

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>

src/controllers/BaseController.sol

@@ -46,13 +46,13 @@ abstract contract BaseController is Ownable, Oval {
      * @param newLockWindow The lockWindow to set.
      */
     function setLockWindow(uint256 newLockWindow) public onlyOwner {
+        require(maxAge() > newLockWindow, "Max age not above lock window");
+
         (int256 currentAnswer, uint256 currentTimestamp,) = internalLatestData();
 
         lockWindow_ = newLockWindow;
 
-        // Compare Oval results so that change in lock window does not change returned data.
-        (int256 newAnswer, uint256 newTimestamp,) = internalLatestData();
-        require(currentAnswer == newAnswer && currentTimestamp == newTimestamp, "Must unlock first");
+        _checkDataNotChanged(currentAnswer, currentTimestamp);
 
         emit LockWindowSet(newLockWindow);
     }
@@ -62,8 +62,14 @@ abstract contract BaseController is Ownable, Oval {
      * @param newMaxTraversal The maxTraversal to set.
      */
     function setMaxTraversal(uint256 newMaxTraversal) public onlyOwner {
+        require(newMaxTraversal > 0, "Max traversal must be > 0");
+
+        (int256 currentAnswer, uint256 currentTimestamp,) = internalLatestData();
+
         maxTraversal_ = newMaxTraversal;
 
+        _checkDataNotChanged(currentAnswer, currentTimestamp);
+
         emit MaxTraversalSet(newMaxTraversal);
     }
 
@@ -72,8 +78,14 @@ abstract contract BaseController is Ownable, Oval {
      * @param newMaxAge The maxAge to set
      */
     function setMaxAge(uint256 newMaxAge) public onlyOwner {
+        require(newMaxAge > lockWindow(), "Max age not above lock window");
+
+        (int256 currentAnswer, uint256 currentTimestamp,) = internalLatestData();
+
         maxAge_ = newMaxAge;
 
+        _checkDataNotChanged(currentAnswer, currentTimestamp);
+
         emit MaxAgeSet(newMaxAge);
     }
 
@@ -101,4 +113,10 @@ abstract contract BaseController is Ownable, Oval {
     function maxAge() public view override returns (uint256) {
         return maxAge_;
     }
+
+    // Helper function to ensure that changing controller parameters does not change the returned data.
+    function _checkDataNotChanged(int256 currentAnswer, uint256 currentTimestamp) internal view {
+        (int256 newAnswer, uint256 newTimestamp,) = internalLatestData();
+        require(currentAnswer == newAnswer && currentTimestamp == newTimestamp, "Must unlock first");
+    }
 }

test/fork/adapters/UnionSourceAdapter.sol

@@ -165,9 +165,9 @@ contract UnionSourceAdapterTest is CommonTest {
     function testLookbackDropChronicle() public {
         // Fork to a block where chronicle was the newest.
         vm.createSelectFork("mainnet", targetChronicleBlock);
+        _whitelistOnChronicle();
         uint256 targetTimestamp = block.timestamp;
         sourceAdapter.setMaxAge(2 days); // Set max age to 2 days to disable this logic for the test.
-        _whitelistOnChronicle();
 
         // Snapshotting union adapter should not affect historical lookups, but we do it just to prove it does not interfere.
         sourceAdapter.snapshotData();

test/unit/BaseController.sol

@@ -16,6 +16,8 @@ contract BaseControllerTest is CommonTest {
     TestBaseController baseController;
 
     function setUp() public {
+        vm.warp(lastUnlockTime);
+
         vm.startPrank(owner);
         baseController = new TestBaseController(18);
         baseController.setUnlocker(permissionedUnlocker, true);

test/unit/SnapshotSource.SnapshotData.sol

@@ -78,38 +78,39 @@ contract SnapshotSourceSnapshotDataTest is CommonTest {
 
     function testMaxAgeIsRespected() public {
         // Set maxAge to 2000 for testing
+        vm.warp(10000);
         snapshotSource.setMaxAge(2000);
 
         // Publish data at different timestamps
-        vm.warp(1000);
-        snapshotSource.publishSourceData(100, 1000);
+        vm.warp(11000);
+        snapshotSource.publishSourceData(100, 11000);
         snapshotSource.snapshotData();
 
-        vm.warp(2000);
-        snapshotSource.publishSourceData(200, 2000);
+        vm.warp(12000);
+        snapshotSource.publishSourceData(200, 12000);
         snapshotSource.snapshotData();
 
-        vm.warp(3000);
-        snapshotSource.publishSourceData(300, 3000);
+        vm.warp(13000);
+        snapshotSource.publishSourceData(300, 13000);
         snapshotSource.snapshotData();
 
-        vm.warp(4000);
-        snapshotSource.publishSourceData(400, 4000);
+        vm.warp(14000);
+        snapshotSource.publishSourceData(400, 14000);
         snapshotSource.snapshotData();
 
         // Verify behavior when requesting data within the maxAge limit
-        (int256 answerAt4000, uint256 timestampAt4000,) = snapshotSource.tryLatestDataAt(4000, 10);
-        assertTrue(answerAt4000 == 400 && timestampAt4000 == 4000);
+        (int256 answerAt14000, uint256 timestampAt14000,) = snapshotSource.tryLatestDataAt(14000, 10);
+        assertTrue(answerAt14000 == 400 && timestampAt14000 == 14000);
 
-        (int256 answerAt3000, uint256 timestampAt3000,) = snapshotSource.tryLatestDataAt(3000, 10);
-        assertTrue(answerAt3000 == 300 && timestampAt3000 == 3000);
+        (int256 answerAt13000, uint256 timestampAt13000,) = snapshotSource.tryLatestDataAt(13000, 10);
+        assertTrue(answerAt13000 == 300 && timestampAt13000 == 13000);
 
         // Request data at the limit of maxAge should still work.
-        (int256 answerAt2000, uint256 timestampAt2000,) = snapshotSource.tryLatestDataAt(2000, 10);
-        assertTrue(answerAt2000 == 200 && timestampAt2000 == 2000);
+        (int256 answerAt12000, uint256 timestampAt12000,) = snapshotSource.tryLatestDataAt(12000, 10);
+        assertTrue(answerAt12000 == 200 && timestampAt12000 == 12000);
 
-        // Request data older than maxAge (1000), should get the latest available data at 4000.
-        (int256 answerAt1000, uint256 timestampAt1000,) = snapshotSource.tryLatestDataAt(1000, 10);
-        assertTrue(answerAt1000 == 400 && timestampAt1000 == 4000);
+        // Request data older than maxAge (1000), should get the latest available data at 14000.
+        (int256 answerAt11000, uint256 timestampAt11000,) = snapshotSource.tryLatestDataAt(11000, 10);
+        assertTrue(answerAt11000 == 400 && timestampAt11000 == 14000);
     }
 }