Skip to content

Commit 549fbfc

Browse files
8sunyuanypatil12
8sunyuan
authored and
ypatil12
committed
fix: try catch out of gas edge case (#971)
1 parent ac24bdd commit 549fbfc

File tree

7 files changed

+250
-20
lines changed

7 files changed

+250
-20
lines changed

‎pkg/bindings/StrategyManager/binding.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎script/releases/v1.0.2-slashing/1-eoa.s.sol

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity ^0.8.12;
3+
4+
import {EOADeployer} from "zeus-templates/templates/EOADeployer.sol";
5+
import "../Env.sol";
6+
7+
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
8+
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
9+
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
10+
import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
11+
12+
// Just upgrade StrategyManager
13+
contract Deploy is EOADeployer {
14+
using Env for *;
15+
16+
function _runAsEOA() internal override {
17+
vm.startBroadcast();
18+
deployImpl({
19+
name: type(StrategyManager).name,
20+
deployedTo: address(new StrategyManager({
21+
_delegation: Env.proxy.delegationManager(),
22+
_pauserRegistry: Env.impl.pauserRegistry()
23+
}))
24+
});
25+
26+
vm.stopBroadcast();
27+
}
28+
29+
function testDeploy() public virtual {
30+
_runAsEOA();
31+
_validateNewImplAddresses(false);
32+
_validateImplConstructors();
33+
_validateImplsInitialized();
34+
}
35+
36+
37+
/// @dev Validate that the `Env.impl` addresses are updated to be distinct from what the proxy
38+
/// admin reports as the current implementation address.
39+
///
40+
/// Note: The upgrade script can call this with `areMatching == true` to check that these impl
41+
/// addresses _are_ matches.
42+
function _validateNewImplAddresses(bool areMatching) internal view {
43+
function (address, address, string memory) internal pure assertion =
44+
areMatching ? _assertMatch : _assertNotMatch;
45+
46+
47+
assertion(
48+
_getProxyImpl(address(Env.proxy.strategyManager())),
49+
address(Env.impl.strategyManager()),
50+
"strategyManager impl failed"
51+
);
52+
}
53+
54+
/// @dev Validate the immutables set in the new implementation constructors
55+
function _validateImplConstructors() internal view {
56+
StrategyManager strategyManager = Env.impl.strategyManager();
57+
assertTrue(strategyManager.delegation() == Env.proxy.delegationManager(), "sm.dm invalid");
58+
assertTrue(strategyManager.pauserRegistry() == Env.impl.pauserRegistry(), "sm.pR invalid");
59+
}
60+
61+
/// @dev Call initialize on all deployed implementations to ensure initializers are disabled
62+
function _validateImplsInitialized() internal {
63+
bytes memory errInit = "Initializable: contract is already initialized";
64+
65+
StrategyManager strategyManager = Env.impl.strategyManager();
66+
vm.expectRevert(errInit);
67+
strategyManager.initialize(address(0), address(0), 0);
68+
}
69+
70+
/// @dev Query and return `proxyAdmin.getProxyImplementation(proxy)`
71+
function _getProxyImpl(address proxy) internal view returns (address) {
72+
return ProxyAdmin(Env.proxyAdmin()).getProxyImplementation(ITransparentUpgradeableProxy(proxy));
73+
}
74+
75+
/// @dev Query and return `proxyAdmin.getProxyAdmin(proxy)`
76+
function _getProxyAdmin(address proxy) internal view returns (address) {
77+
return ProxyAdmin(Env.proxyAdmin()).getProxyAdmin(ITransparentUpgradeableProxy(proxy));
78+
}
79+
80+
function _assertMatch(address a, address b, string memory err) private pure {
81+
assertEq(a, b, err);
82+
}
83+
84+
function _assertNotMatch(address a, address b, string memory err) private pure {
85+
assertNotEq(a, b, err);
86+
}
87+
}

‎script/releases/v1.0.2-slashing/2-multisig.s.sol

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity ^0.8.12;
3+
4+
import {Deploy} from "./1-eoa.s.sol";
5+
import "../Env.sol";
6+
7+
import {MultisigBuilder} from "zeus-templates/templates/MultisigBuilder.sol";
8+
import "zeus-templates/utils/Encode.sol";
9+
10+
import {TimelockController} from "@openzeppelin/contracts/governance/TimelockController.sol";
11+
12+
contract Queue is MultisigBuilder, Deploy {
13+
using Env for *;
14+
using Encode for *;
15+
16+
function _runAsMultisig() prank(Env.opsMultisig()) internal virtual override {
17+
bytes memory calldata_to_executor = _getCalldataToExecutor();
18+
19+
TimelockController timelock = Env.timelockController();
20+
timelock.schedule({
21+
target: Env.executorMultisig(),
22+
value: 0,
23+
data: calldata_to_executor,
24+
predecessor: 0,
25+
salt: 0,
26+
delay: timelock.getMinDelay()
27+
});
28+
}
29+
30+
/// @dev Get the calldata to be sent from the timelock to the executor
31+
function _getCalldataToExecutor() internal returns (bytes memory) {
32+
MultisigCall[] storage executorCalls = Encode.newMultisigCalls()
33+
/// core/
34+
.append({
35+
to: Env.proxyAdmin(),
36+
data: Encode.proxyAdmin.upgrade({
37+
proxy: address(Env.proxy.strategyManager()),
38+
impl: address(Env.impl.strategyManager())
39+
})
40+
});
41+
42+
return Encode.gnosisSafe.execTransaction({
43+
from: address(Env.timelockController()),
44+
to: address(Env.multiSendCallOnly()),
45+
op: Encode.Operation.DelegateCall,
46+
data: Encode.multiSend(executorCalls)
47+
});
48+
}
49+
50+
function testScript() public virtual {
51+
runAsEOA();
52+
53+
TimelockController timelock = Env.timelockController();
54+
bytes memory calldata_to_executor = _getCalldataToExecutor();
55+
bytes32 txHash = timelock.hashOperation({
56+
target: Env.executorMultisig(),
57+
value: 0,
58+
data: calldata_to_executor,
59+
predecessor: 0,
60+
salt: 0
61+
});
62+
63+
// Check that the upgrade does not exist in the timelock
64+
assertFalse(timelock.isOperationPending(txHash), "Transaction should NOT be queued.");
65+
66+
execute();
67+
68+
// Check that the upgrade has been added to the timelock
69+
assertTrue(timelock.isOperationPending(txHash), "Transaction should be queued.");
70+
}
71+
}

‎script/releases/v1.0.2-slashing/3-execute.s.sol

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// SPDX-License-Identifier: BUSL-1.1
2+
pragma solidity ^0.8.12;
3+
4+
import "../Env.sol";
5+
import {Queue} from "./2-multisig.s.sol";
6+
7+
import "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
8+
import "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
9+
10+
contract Execute is Queue {
11+
using Env for *;
12+
13+
function _runAsMultisig() prank(Env.protocolCouncilMultisig()) internal override(Queue) {
14+
bytes memory calldata_to_executor = _getCalldataToExecutor();
15+
16+
TimelockController timelock = Env.timelockController();
17+
timelock.execute({
18+
target: Env.executorMultisig(),
19+
value: 0,
20+
payload: calldata_to_executor,
21+
predecessor: 0,
22+
salt: 0
23+
});
24+
}
25+
26+
function testScript() public virtual override(Queue){
27+
// 0. Deploy Impls
28+
runAsEOA();
29+
30+
TimelockController timelock = Env.timelockController();
31+
bytes memory calldata_to_executor = _getCalldataToExecutor();
32+
bytes32 txHash = timelock.hashOperation({
33+
target: Env.executorMultisig(),
34+
value: 0,
35+
data: calldata_to_executor,
36+
predecessor: 0,
37+
salt: 0
38+
});
39+
assertFalse(timelock.isOperationPending(txHash), "Transaction should NOT be queued.");
40+
41+
// 1. Queue Upgrade
42+
Queue._runAsMultisig();
43+
_unsafeResetHasPranked(); // reset hasPranked so we can use it again
44+
45+
// 2. Warp past delay
46+
vm.warp(block.timestamp + timelock.getMinDelay()); // 1 tick after ETA
47+
assertEq(timelock.isOperationReady(txHash), true, "Transaction should be executable.");
48+
49+
// 3- execute
50+
execute();
51+
52+
assertTrue(timelock.isOperationDone(txHash), "Transaction should be complete.");
53+
54+
// 4. Validate
55+
_validateNewImplAddresses(true);
56+
_validateProxyConstructors();
57+
}
58+
59+
function _validateProxyConstructors() internal view {
60+
StrategyManager strategyManager = Env.proxy.strategyManager();
61+
assertTrue(strategyManager.delegation() == Env.proxy.delegationManager(), "sm.dm invalid");
62+
assertTrue(strategyManager.pauserRegistry() == Env.impl.pauserRegistry(), "sm.pR invalid");
63+
}
64+
}

‎script/releases/v1.0.2-slashing/upgrade.json

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
{
2+
"name": "slashing-patch",
3+
"from": "1.0.1",
4+
"to": "1.0.2",
5+
"phases": [
6+
{
7+
"type": "eoa",
8+
"filename": "1-eoa.s.sol"
9+
},
10+
{
11+
"type": "multisig",
12+
"filename": "2-multisig.s.sol"
13+
},
14+
{
15+
"type": "multisig",
16+
"filename": "3-execute.s.sol"
17+
}
18+
]
19+
}

‎src/contracts/core/StrategyManager.sol

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ contract StrategyManager is
143143
/// @inheritdoc IStrategyManager
144144
function burnShares(IStrategy strategy, uint256 sharesToBurn) external onlyDelegationManager {
145145
// burning shares is functionally the same as withdrawing but with different destination address
146-
try strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn) {} catch {}
146+
strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn);
147147
}
148148

149149
/// @inheritdoc IStrategyManager

‎src/test/unit/StrategyManagerUnit.t.sol

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1421,6 +1421,7 @@ contract StrategyManagerUnitTests_withdrawSharesAsTokens is StrategyManagerUnitT
14211421
) external filterFuzzedAddressInputs(staker) {
14221422
cheats.assume(staker != address(this));
14231423
cheats.assume(staker != address(0));
1424+
cheats.assume(staker != address(dummyStrat));
14241425
cheats.assume(depositAmount > 0 && depositAmount < dummyToken.totalSupply() && depositAmount < sharesAmount);
14251426
IStrategy strategy = dummyStrat;
14261427
IERC20 token = dummyToken;
@@ -1457,10 +1458,10 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
14571458
}
14581459

14591460
/**
1460-
* @notice deposits a single strategy and withdrawSharesAsTokens() function reverts when sharesAmount is
1461-
* higher than depositAmount
1461+
* @notice deposits a single strategy and withdrawSharesAsTokens(). Tests that we revert when we
1462+
* burn more than expected
14621463
*/
1463-
function testFuzz_ShareAmountTooHigh(
1464+
function testFuzz_RevertShareAmountTooHigh(
14641465
address staker,
14651466
uint256 depositAmount,
14661467
uint256 sharesToBurn
@@ -1471,15 +1472,9 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
14711472
IERC20 token = dummyToken;
14721473
_depositIntoStrategySuccessfully(strategy, staker, depositAmount);
14731474

1474-
uint256 strategyBalanceBefore = token.balanceOf(address(strategy));
1475-
uint256 burnAddressBalanceBefore = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
14761475
cheats.prank(address(delegationManagerMock));
1476+
cheats.expectRevert(IStrategyErrors.WithdrawalAmountExceedsTotalDeposits.selector);
14771477
strategyManager.burnShares(strategy, sharesToBurn);
1478-
uint256 strategyBalanceAfter = token.balanceOf(address(strategy));
1479-
uint256 burnAddressBalanceAfter = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
1480-
1481-
assertEq(burnAddressBalanceBefore, burnAddressBalanceAfter, "burnAddressBalanceBefore != burnAddressBalanceAfter");
1482-
assertEq(strategyBalanceBefore, strategyBalanceAfter, "strategyBalanceBefore != strategyBalanceAfter");
14831478
}
14841479

14851480
function testFuzz_SingleStrategyDeposited(
@@ -1518,7 +1513,7 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
15181513
}
15191514

15201515
/// @notice check that balances are unchanged with a reverting token but burnShares doesn't revert
1521-
function testFuzz_tryCatchWithRevertToken(
1516+
function testFuzz_revertTryCatchWithRevertToken(
15221517
address staker,
15231518
uint256 depositAmount,
15241519
uint256 sharesToBurn
@@ -1533,15 +1528,9 @@ contract StrategyManagerUnitTests_burnShares is StrategyManagerUnitTests {
15331528
cheats.etch(address(token), address(revertToken).code);
15341529
ERC20_SetTransferReverting_Mock(address(token)).setTransfersRevert(true);
15351530

1536-
uint256 strategyBalanceBefore = token.balanceOf(address(strategy));
1537-
uint256 burnAddressBalanceBefore = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
1531+
cheats.expectRevert("SafeERC20: low-level call failed");
15381532
cheats.prank(address(delegationManagerMock));
15391533
strategyManager.burnShares(strategy, sharesToBurn);
1540-
uint256 strategyBalanceAfter = token.balanceOf(address(strategy));
1541-
uint256 burnAddressBalanceAfter = token.balanceOf(strategyManager.DEFAULT_BURN_ADDRESS());
1542-
1543-
assertEq(burnAddressBalanceBefore, burnAddressBalanceAfter, "burnAddressBalanceBefore != burnAddressBalanceAfter");
1544-
assertEq(strategyBalanceBefore, strategyBalanceAfter, "strategyBalanceBefore != strategyBalanceAfter");
15451534
}
15461535
}
15471536

0 commit comments

Comments
 (0)