Skip to content

Commit c920c19

Browse files
riccardo-ssvlabsriccardo-ssvlabs
committed
feat: <- uniform timestamps and unique check
1 parent bd55fb0 commit c920c19

File tree

3 files changed

+41
-47
lines changed

3 files changed

+41
-47
lines changed

‎src/BasedAppManager.sol‎

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -440,10 +440,7 @@ contract BasedAppManager is
440440
uint256 requestTime = request.requestTime;
441441

442442
if (requestTime == 0) revert ICore.NoPendingWithdrawal();
443-
if (block.timestamp < requestTime + WITHDRAWAL_TIMELOCK_PERIOD) revert ICore.WithdrawalTimelockNotElapsed();
444-
if (block.timestamp > requestTime + WITHDRAWAL_TIMELOCK_PERIOD + WITHDRAWAL_EXPIRE_TIME) {
445-
revert ICore.WithdrawalExpired();
446-
}
443+
_checkTimelocks(requestTime, WITHDRAWAL_TIMELOCK_PERIOD, WITHDRAWAL_EXPIRE_TIME);
447444

448445
uint256 amount = request.amount;
449446
strategyTokenBalances[strategyId][msg.sender][address(token)] -= amount;
@@ -480,10 +477,7 @@ contract BasedAppManager is
480477
uint256 requestTime = request.requestTime;
481478

482479
if (requestTime == 0) revert ICore.NoPendingWithdrawalETH();
483-
if (block.timestamp < requestTime + WITHDRAWAL_TIMELOCK_PERIOD) revert ICore.WithdrawalTimelockNotElapsed();
484-
if (block.timestamp > requestTime + WITHDRAWAL_TIMELOCK_PERIOD + WITHDRAWAL_EXPIRE_TIME) {
485-
revert ICore.WithdrawalExpired();
486-
}
480+
_checkTimelocks(requestTime, WITHDRAWAL_TIMELOCK_PERIOD, WITHDRAWAL_EXPIRE_TIME);
487481

488482
uint256 amount = request.amount;
489483
strategyTokenBalances[strategyId][msg.sender][ETH_ADDRESS] -= amount;
@@ -570,12 +564,7 @@ contract BasedAppManager is
570564
uint32 percentage = request.percentage;
571565

572566
if (requestTime == 0) revert ICore.NoPendingObligationUpdate();
573-
if (block.timestamp < request.requestTime + OBLIGATION_TIMELOCK_PERIOD) {
574-
revert ICore.ObligationTimelockNotElapsed();
575-
}
576-
if (block.timestamp > request.requestTime + OBLIGATION_TIMELOCK_PERIOD + OBLIGATION_EXPIRE_TIME) {
577-
revert ICore.UpdateObligationExpired();
578-
}
567+
_checkTimelocks(requestTime, OBLIGATION_TIMELOCK_PERIOD, OBLIGATION_EXPIRE_TIME);
579568

580569
if (percentage == 0 && obligations[strategyId][bApp][address(token)].percentage > 0) {
581570
usedTokens[strategyId][address(token)] -= 1;
@@ -601,9 +590,9 @@ contract BasedAppManager is
601590
if (proposedFee == fee) revert ICore.FeeAlreadySet();
602591

603592
strategy.feeProposed = proposedFee;
604-
strategy.feeUpdateTime = block.timestamp + FEE_TIMELOCK_PERIOD;
593+
strategy.feeRequestTime = block.timestamp;
605594

606-
emit StrategyFeeUpdateProposed(strategyId, msg.sender, proposedFee, fee, strategy.feeUpdateTime);
595+
emit StrategyFeeUpdateProposed(strategyId, msg.sender, proposedFee, fee, strategy.feeRequestTime);
607596
}
608597

609598
/// @notice Finalize the fee update for a strategy
@@ -613,18 +602,15 @@ contract BasedAppManager is
613602
) external onlyStrategyOwner(strategyId) {
614603
ICore.Strategy storage strategy = strategies[strategyId];
615604

616-
uint256 feeUpdateTime = strategy.feeUpdateTime;
605+
uint256 feeRequestTime = strategy.feeRequestTime;
617606

618-
if (feeUpdateTime == 0) revert ICore.NoPendingFeeUpdate();
619-
if (block.timestamp < feeUpdateTime) revert ICore.FeeTimelockNotElapsed();
620-
if (block.timestamp > feeUpdateTime + FEE_EXPIRE_TIME) {
621-
revert ICore.FeeUpdateExpired();
622-
}
607+
if (feeRequestTime == 0) revert ICore.NoPendingFeeUpdate();
608+
_checkTimelocks(feeRequestTime, FEE_TIMELOCK_PERIOD, FEE_EXPIRE_TIME);
623609

624610
uint32 oldFee = strategy.fee;
625611
strategy.fee = strategy.feeProposed;
626612
strategy.feeProposed = 0;
627-
strategy.feeUpdateTime = 0;
613+
strategy.feeRequestTime = 0;
628614

629615
emit StrategyFeeUpdated(strategyId, msg.sender, strategy.fee, oldFee);
630616
}
@@ -740,4 +726,15 @@ contract BasedAppManager is
740726
}
741727
obligations[strategyId][bApp][token].percentage = obligationPercentage;
742728
}
729+
730+
/// @notice Check the timelocks
731+
/// @param requestTime The time of the request
732+
/// @param timelockPeriod The timelock period
733+
/// @param expireTime The expire time
734+
function _checkTimelocks(uint256 requestTime, uint256 timelockPeriod, uint256 expireTime) private view {
735+
if (block.timestamp < requestTime + timelockPeriod) revert ICore.TimelockNotElapsed();
736+
if (block.timestamp > requestTime + timelockPeriod + expireTime) {
737+
revert ICore.RequestTimeExpired();
738+
}
739+
}
743740
}

‎src/interfaces/ICore.sol‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,23 @@ interface ICore {
2626
uint32 fee;
2727
/// @dev The proposed fee
2828
uint32 feeProposed;
29-
/// @dev The proposed fee expiration time
30-
uint256 feeUpdateTime;
29+
/// @dev The block time when the fee update request was sent
30+
uint256 feeRequestTime;
3131
}
3232

3333
/// @notice Represents a request for a withdrawal from a participant of a strategy
3434
struct WithdrawalRequest {
3535
/// @dev The amount requested to withdraw
3636
uint256 amount;
37-
/// @dev The block time when the request was sent
37+
/// @dev The block time when the withdrawal request was sent
3838
uint256 requestTime;
3939
}
4040

4141
/// @notice Represents a change in the obligation in a strategy. Only the owner can submit one.
4242
struct ObligationRequest {
4343
/// @dev The new obligation percentage
4444
uint32 percentage;
45-
/// @dev The block time when the request was sent
45+
/// @dev The block time when the update obligation request was sent
4646
uint256 requestTime;
4747
}
4848

@@ -55,33 +55,30 @@ interface ICore {
5555
error EmptyTokenList();
5656
error ExceedingPercentageUpdate();
5757
error FeeAlreadySet();
58-
error FeeTimelockNotElapsed();
59-
error FeeUpdateExpired();
6058
error InsufficientBalance();
6159
error InvalidAmount();
62-
error InvalidStrategyFee();
60+
error InvalidBAppOwner(address caller, address expectedOwner);
6361
error InvalidMaxFeeIncrement();
6462
error InvalidPercentage();
6563
error InvalidPercentageIncrement();
6664
error InvalidSharedRiskLevel();
65+
error InvalidStrategyFee();
6766
error InvalidStrategyOwner(address caller, address expectedOwner);
68-
error InvalidBAppOwner(address caller, address expectedOwner);
6967
error InvalidToken();
68+
error LengthsNotMatching();
7069
error NoPendingFeeUpdate();
7170
error NoPendingObligationUpdate();
7271
error NoPendingWithdrawal();
7372
error NoPendingWithdrawalETH();
7473
error ObligationAlreadySet();
7574
error ObligationHasNotBeenCreated();
76-
error ObligationTimelockNotElapsed();
7775
error PercentageAlreadySet();
76+
error RequestTimeExpired();
7877
error SharedRiskLevelAlreadySet();
78+
error TimelockNotElapsed();
7979
error TokenAlreadyAddedToBApp(address token);
8080
error TokenIsUsedByTheBApp();
8181
error TokenNoTSupportedByBApp(address token);
82-
error LengthsNotMatching();
8382
error UpdateObligationExpired();
84-
error WithdrawalExpired();
85-
error WithdrawalTimelockNotElapsed();
8683
error ZeroAddressNotAllowed();
8784
}

‎test/BAppManager.strategy.t.sol‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -727,7 +727,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
727727
assertEq(owner, USER1, "Strategy owner");
728728
assertEq(fee, STRATEGY1_INITIAL_FEE, "Strategy fee");
729729
assertEq(feeProposed, 20, "Strategy fee proposed");
730-
assertEq(feeUpdateTime, 604_801, "Strategy fee update time");
730+
assertEq(feeUpdateTime, 1, "Strategy fee update time");
731731
vm.warp(block.timestamp + proxiedManager.FEE_TIMELOCK_PERIOD() + timeBeforeLimit);
732732
proxiedManager.finalizeFeeUpdate(STRATEGY1);
733733
(owner, fee, feeProposed, feeUpdateTime) = proxiedManager.strategies(STRATEGY1);
@@ -749,9 +749,9 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
749749
assertEq(owner, USER1, "Strategy owner");
750750
assertEq(fee, STRATEGY1_INITIAL_FEE, "Strategy fee");
751751
assertEq(feeProposed, 20, "Strategy fee proposed");
752-
assertEq(feeUpdateTime, 604_801, "Strategy fee update time");
752+
assertEq(feeUpdateTime, 1, "Strategy fee update time");
753753
vm.warp(block.timestamp + proxiedManager.FEE_TIMELOCK_PERIOD() + timeAfterLimit);
754-
vm.expectRevert(abi.encodeWithSelector(ICore.FeeUpdateExpired.selector));
754+
vm.expectRevert(abi.encodeWithSelector(ICore.RequestTimeExpired.selector));
755755
proxiedManager.finalizeFeeUpdate(STRATEGY1);
756756
vm.stopPrank();
757757
}
@@ -764,9 +764,9 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
764764
assertEq(owner, USER1, "Strategy owner");
765765
assertEq(fee, STRATEGY1_INITIAL_FEE, "Strategy fee");
766766
assertEq(feeProposed, 20, "Strategy fee proposed");
767-
assertEq(feeUpdateTime, 604_801, "Strategy fee update time");
767+
assertEq(feeUpdateTime, 1, "Strategy fee update time");
768768
vm.warp(block.timestamp + proxiedManager.FEE_TIMELOCK_PERIOD() - 1 seconds);
769-
vm.expectRevert(abi.encodeWithSelector(ICore.FeeTimelockNotElapsed.selector));
769+
vm.expectRevert(abi.encodeWithSelector(ICore.TimelockNotElapsed.selector));
770770
proxiedManager.finalizeFeeUpdate(STRATEGY1);
771771
vm.stopPrank();
772772
}
@@ -798,7 +798,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
798798
assertEq(owner, USER1, "Strategy owner");
799799
assertEq(fee, STRATEGY1_INITIAL_FEE, "Strategy fee");
800800
assertEq(feeProposed, 20, "Strategy fee proposed");
801-
assertEq(feeUpdateTime, 604_801, "Strategy fee update time");
801+
assertEq(feeUpdateTime, 1, "Strategy fee update time");
802802
vm.warp(block.timestamp + proxiedManager.FEE_TIMELOCK_PERIOD());
803803
vm.stopPrank();
804804
vm.startPrank(ATTACKER);
@@ -914,7 +914,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
914914
assertEq(percentage, 1000, "Obligation percentage proposed");
915915
assertEq(requestTime, 1, "Obligation update time");
916916
vm.warp(block.timestamp + proxiedManager.OBLIGATION_TIMELOCK_PERIOD() + timeAfterLimit);
917-
vm.expectRevert(abi.encodeWithSelector(ICore.UpdateObligationExpired.selector));
917+
vm.expectRevert(abi.encodeWithSelector(ICore.RequestTimeExpired.selector));
918918
proxiedManager.finalizeUpdateObligation(STRATEGY1, BAPP1, address(erc20mock));
919919
vm.stopPrank();
920920
}
@@ -934,7 +934,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
934934
assertEq(percentage, 1000, "Obligation percentage proposed");
935935
assertEq(requestTime, 1, "Obligation update time");
936936
vm.warp(block.timestamp + proxiedManager.OBLIGATION_TIMELOCK_PERIOD() - timeToLimit);
937-
vm.expectRevert(abi.encodeWithSelector(ICore.ObligationTimelockNotElapsed.selector));
937+
vm.expectRevert(abi.encodeWithSelector(ICore.TimelockNotElapsed.selector));
938938
proxiedManager.finalizeUpdateObligation(STRATEGY1, BAPP1, address(erc20mock));
939939
vm.stopPrank();
940940
}
@@ -1058,7 +1058,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
10581058
assertEq(requestTime, block.timestamp, "Request time");
10591059
assertEq(amount, withdrawalAmount, "Request amount");
10601060
vm.warp(block.timestamp + proxiedManager.WITHDRAWAL_TIMELOCK_PERIOD() - 1 seconds);
1061-
vm.expectRevert(abi.encodeWithSelector(ICore.WithdrawalTimelockNotElapsed.selector));
1061+
vm.expectRevert(abi.encodeWithSelector(ICore.TimelockNotElapsed.selector));
10621062
proxiedManager.finalizeWithdrawalETH(STRATEGY1);
10631063
vm.stopPrank();
10641064
}
@@ -1082,7 +1082,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
10821082
block.timestamp + proxiedManager.WITHDRAWAL_TIMELOCK_PERIOD() + proxiedManager.WITHDRAWAL_EXPIRE_TIME()
10831083
+ 1 seconds
10841084
);
1085-
vm.expectRevert(abi.encodeWithSelector(ICore.WithdrawalExpired.selector));
1085+
vm.expectRevert(abi.encodeWithSelector(ICore.RequestTimeExpired.selector));
10861086
proxiedManager.finalizeWithdrawalETH(STRATEGY1);
10871087
vm.stopPrank();
10881088
}
@@ -1111,7 +1111,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
11111111
assertEq(requestTime, block.timestamp, "Request time");
11121112
assertEq(amount, 1000, "Request amount");
11131113
vm.warp(block.timestamp + proxiedManager.WITHDRAWAL_TIMELOCK_PERIOD() - 1 seconds);
1114-
vm.expectRevert(abi.encodeWithSelector(ICore.WithdrawalTimelockNotElapsed.selector));
1114+
vm.expectRevert(abi.encodeWithSelector(ICore.TimelockNotElapsed.selector));
11151115
proxiedManager.finalizeWithdrawal(STRATEGY1, erc20mock);
11161116
vm.stopPrank();
11171117
}
@@ -1132,7 +1132,7 @@ contract BasedAppManagerStrategyTest is BasedAppManagerSetupTest, BasedAppManage
11321132
block.timestamp + proxiedManager.WITHDRAWAL_TIMELOCK_PERIOD() + proxiedManager.WITHDRAWAL_EXPIRE_TIME()
11331133
+ 1 seconds
11341134
);
1135-
vm.expectRevert(abi.encodeWithSelector(ICore.WithdrawalExpired.selector));
1135+
vm.expectRevert(abi.encodeWithSelector(ICore.RequestTimeExpired.selector));
11361136
proxiedManager.finalizeWithdrawal(STRATEGY1, erc20mock);
11371137
vm.stopPrank();
11381138
}

0 commit comments

Comments
 (0)