Skip to content

Commit

Permalink
fix: dont call strategy if we have no burnable shares (#1106)
Browse files Browse the repository at this point in the history
**Motivation:**

Fixes an issue arbitrary external contracts could be called via
`StrategyManager.burnShares`. (Certora L-04)

**Modifications:**

`StrategyManager.burnShares` does not do an external call if the
burnable share amount is zero

**Result:**

Should no longer be possible to call untrusted code directly through
`burnShares`
  • Loading branch information
wadealexc authored Feb 14, 2025
1 parent 81fef63 commit 319d232
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 2 deletions.
2 changes: 2 additions & 0 deletions docs/core/StrategyManager.md
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,8 @@ function burnShares(

Anyone can call this method to burn slashed shares previously added by the `DelegationManager` via `increaseBurnableShares`. This method resets the strategy's burnable shares to 0, and directs the corresponding `strategy` to convert the shares to tokens and transfer them to `DEFAULT_BURN_ADDRESS`, rendering them unrecoverable.

The `strategy` is not called if the strategy had no burnable shares.

*Effects*:
* Resets the strategy's burnable shares to 0
* Calls `withdraw` on the `strategy`, withdrawing shares and sending a corresponding amount of tokens to the `DEFAULT_BURN_ADDRESS`
Expand Down
8 changes: 6 additions & 2 deletions src/contracts/core/StrategyManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,12 @@ contract StrategyManager is
(, uint256 sharesToBurn) = EnumerableMap.tryGet(burnableShares, address(strategy));
EnumerableMap.remove(burnableShares, address(strategy));
emit BurnableSharesDecreased(strategy, sharesToBurn);
// burning shares is functionally the same as withdrawing but with different destination address
strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn);

// Burning acts like withdrawing, except that the destination is to the burn address.
// If we have no shares to burn, we don't need to call the strategy.
if (sharesToBurn != 0) {
strategy.withdraw(DEFAULT_BURN_ADDRESS, strategy.underlyingToken(), sharesToBurn);
}
}

/// @inheritdoc IStrategyManager
Expand Down

0 comments on commit 319d232

Please sign in to comment.