From 71c59c9ded59f5a0cf972b3168f782f5a61e5c5d Mon Sep 17 00:00:00 2001 From: Reinis Martinsons Date: Mon, 17 Jun 2024 11:46:48 +0000 Subject: [PATCH 1/2] fix[oval-audit-m-04]: validate base controller parameters Signed-off-by: Reinis Martinsons --- src/controllers/BaseController.sol | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/controllers/BaseController.sol b/src/controllers/BaseController.sol index 36a30dd..16b4246 100644 --- a/src/controllers/BaseController.sol +++ b/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"); + } } From eed52a9c3e77fdbce37e90832a535ab1d555645d Mon Sep 17 00:00:00 2001 From: Reinis Martinsons Date: Mon, 17 Jun 2024 12:28:59 +0000 Subject: [PATCH 2/2] fix tests Signed-off-by: Reinis Martinsons --- test/fork/adapters/UnionSourceAdapter.sol | 2 +- test/unit/BaseController.sol | 2 ++ test/unit/SnapshotSource.SnapshotData.sol | 35 ++++++++++++----------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/test/fork/adapters/UnionSourceAdapter.sol b/test/fork/adapters/UnionSourceAdapter.sol index ca0e14b..e04c80e 100644 --- a/test/fork/adapters/UnionSourceAdapter.sol +++ b/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(); diff --git a/test/unit/BaseController.sol b/test/unit/BaseController.sol index 7a2083c..b00abe6 100644 --- a/test/unit/BaseController.sol +++ b/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); diff --git a/test/unit/SnapshotSource.SnapshotData.sol b/test/unit/SnapshotSource.SnapshotData.sol index f5b501f..576e939 100644 --- a/test/unit/SnapshotSource.SnapshotData.sol +++ b/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); } }