Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(chore): Parameterize the BalanceTracker version #123

Closed
wants to merge 1 commit into from
Closed

(chore): Parameterize the BalanceTracker version #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/revenue-share/BalanceTracker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ import {SafeCall} from "@eth-optimism-bedrock/src/libraries/SafeCall.sol";
*/
contract BalanceTracker is ReentrancyGuardUpgradeable {
using Address for address;

/*//////////////////////////////////////////////////////////////
Constants
//////////////////////////////////////////////////////////////*/
/**
* @dev The maximum number of system addresses that can be funded.
*/

uint256 public constant MAX_SYSTEM_ADDRESS_COUNT = 20;

/*//////////////////////////////////////////////////////////////
Expand All @@ -37,6 +37,7 @@ contract BalanceTracker is ReentrancyGuardUpgradeable {
* @dev The system addresses being funded.
*/
address payable[] public systemAddresses;

/**
* @dev The target balances for system addresses.
*/
Expand All @@ -55,13 +56,15 @@ contract BalanceTracker is ReentrancyGuardUpgradeable {
event ProcessedFunds(
address indexed _systemAddress, bool indexed _success, uint256 _balanceNeeded, uint256 _balanceSent
);

/**
* @dev Emitted when the BalanceTracker attempts to send funds to the profit wallet.
* @param _profitWallet The address of the profit wallet.
* @param _success A boolean denoting the success or failure of fund send.
* @param _balanceSent The amount of funds sent to the profit wallet.
*/
event SentProfit(address indexed _profitWallet, bool indexed _success, uint256 _balanceSent);

/**
* @dev Emitted when funds are received.
* @param _sender The address sending funds.
Expand Down Expand Up @@ -91,10 +94,11 @@ contract BalanceTracker is ReentrancyGuardUpgradeable {
* @dev Initializes the BalanceTracker contract.
* @param _systemAddresses The system addresses being funded.
* @param _targetBalances The target balances for system addresses.
* @param _version The version of the contract being initialized. Each new version must be greater than the previous version.
*/
function initialize(address payable[] memory _systemAddresses, uint256[] memory _targetBalances)
function initialize(address payable[] memory _systemAddresses, uint256[] memory _targetBalances, uint8 _version)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not see any access control on the initialize() function. Now that _version is a user-supplied parameter, this seems to allow arbitrary reinitialization by any caller who provides a higher version number.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, perhaps we want a way to just change the addresses without having to call initialize

Copy link

@cbfyi cbfyi Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if this was hardcoded (like before this PR), it'd have to be incremented every time we need to update the addresses, but nothing about the core implementation actually changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that the previous reinitializer(2) prevented it from being reinitialized. Now that it uses reinitializer(_version), I think anyone could reinitialize the contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, perhaps we want a way to just change the addresses without having to call initialize

I think we would still need to enforce access control on who can update those addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow yeah good catch. So you think we should make it ownable and add a setter function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not worth the trouble actually - inheriting a new contract could screw up the storage layout making it not feasible to upgrade. We'd have to add custom logic

external
reinitializer(2)
reinitializer(_version)
{
uint256 systemAddresesLength = _systemAddresses.length;
require(systemAddresesLength > 0, "BalanceTracker: systemAddresses cannot have a length of zero");
Expand Down Expand Up @@ -129,7 +133,7 @@ contract BalanceTracker is ReentrancyGuardUpgradeable {
require(systemAddresesLength > 0, "BalanceTracker: systemAddresses cannot have a length of zero");
// Refills balances of systems addresses up to their target balances
for (uint256 i; i < systemAddresesLength;) {
refillBalanceIfNeeded(systemAddresses[i], targetBalances[i]);
_refillBalanceIfNeeded(systemAddresses[i], targetBalances[i]);
unchecked {
i++;
}
Expand Down Expand Up @@ -157,7 +161,7 @@ contract BalanceTracker is ReentrancyGuardUpgradeable {
* @param _systemAddress The system address being funded.
* @param _targetBalance The target balance for the system address being funded.
*/
function refillBalanceIfNeeded(address _systemAddress, uint256 _targetBalance) internal {
function _refillBalanceIfNeeded(address _systemAddress, uint256 _targetBalance) internal {
uint256 systemAddressBalance = _systemAddress.balance;
if (systemAddressBalance >= _targetBalance) {
emit ProcessedFunds(_systemAddress, false, 0, 0);
Expand Down
27 changes: 14 additions & 13 deletions test/revenue-share/BalanceTracker.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ contract BalanceTrackerTest is CommonTest {
event SentProfit(address indexed _profitWallet, bool indexed _success, uint256 _balanceSent);
event ReceivedFunds(address indexed _sender, uint256 _amount);

uint8 constant VERSION = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to set it to 2 to match what was in the contract previously?

uint256 constant MAX_SYSTEM_ADDRESS_COUNT = 20;
uint256 constant INITIAL_BALANCE_TRACKER_BALANCE = 2_000 ether;

Expand Down Expand Up @@ -58,7 +59,7 @@ contract BalanceTrackerTest is CommonTest {
function test_initializer_fail_systemAddresses_zeroLength() external {
delete systemAddresses;
vm.expectRevert("BalanceTracker: systemAddresses cannot have a length of zero");
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
}

function test_initializer_fail_systemAddresses_greaterThanMaxLength() external {
Expand All @@ -67,32 +68,32 @@ contract BalanceTrackerTest is CommonTest {
}

vm.expectRevert("BalanceTracker: systemAddresses cannot have a length greater than 20");
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
}

function test_initializer_fail_systemAddresses_lengthNotEqualToTargetBalancesLength() external {
systemAddresses.push(payable(address(0)));

vm.expectRevert("BalanceTracker: systemAddresses and targetBalances length must be equal");
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
}

function test_initializer_fail_systemAddresses_containsZeroAddress() external {
systemAddresses[1] = payable(address(0));

vm.expectRevert("BalanceTracker: systemAddresses cannot contain address(0)");
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
}

function test_initializer_fail_targetBalances_containsZero() external {
targetBalances[1] = ZERO_VALUE;

vm.expectRevert("BalanceTracker: targetBalances cannot contain 0 target");
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
}

function test_initializer_success() external {
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);

assertEq(balanceTracker.systemAddresses(0), systemAddresses[0]);
assertEq(balanceTracker.systemAddresses(1), systemAddresses[1]);
Expand All @@ -105,7 +106,7 @@ contract BalanceTrackerTest is CommonTest {
uint256 expectedProfitWalletBalance = INITIAL_BALANCE_TRACKER_BALANCE - l2OutputProposerTargetBalance;
address payable reentrancySystemAddress = payable(address(new ReenterProcessFees()));
systemAddresses[0] = reentrancySystemAddress;
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);

vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(reentrancySystemAddress, false, batchSenderTargetBalance, batchSenderTargetBalance);
Expand All @@ -131,7 +132,7 @@ contract BalanceTrackerTest is CommonTest {
function test_processFees_success_continuesWhenSystemAddressReverts() external {
vm.deal(address(balanceTracker), INITIAL_BALANCE_TRACKER_BALANCE);
uint256 expectedProfitWalletBalance = INITIAL_BALANCE_TRACKER_BALANCE - l2OutputProposerTargetBalance;
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
vm.mockCallRevert(batchSender, bytes(""), abi.encode("revert message"));
vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(batchSender, false, batchSenderTargetBalance, batchSenderTargetBalance);
Expand All @@ -152,7 +153,7 @@ contract BalanceTrackerTest is CommonTest {
vm.deal(address(balanceTracker), INITIAL_BALANCE_TRACKER_BALANCE);
uint256 expectedProfitWalletBalance =
INITIAL_BALANCE_TRACKER_BALANCE - batchSenderTargetBalance - l2OutputProposerTargetBalance;
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(batchSender, true, batchSenderTargetBalance, batchSenderTargetBalance);
vm.expectEmit(true, true, true, true, address(balanceTracker));
Expand All @@ -169,7 +170,7 @@ contract BalanceTrackerTest is CommonTest {
}

function test_processFees_success_noFunds() external {
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(batchSender, true, batchSenderTargetBalance, ZERO_VALUE);
vm.expectEmit(true, true, true, true, address(balanceTracker));
Expand All @@ -188,7 +189,7 @@ contract BalanceTrackerTest is CommonTest {
function test_processFees_success_partialFunds() external {
uint256 partialBalanceTrackerBalance = INITIAL_BALANCE_TRACKER_BALANCE / 3;
vm.deal(address(balanceTracker), partialBalanceTrackerBalance);
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(batchSender, true, batchSenderTargetBalance, partialBalanceTrackerBalance);
vm.expectEmit(true, true, true, true, address(balanceTracker));
Expand All @@ -208,7 +209,7 @@ contract BalanceTrackerTest is CommonTest {
vm.deal(address(balanceTracker), INITIAL_BALANCE_TRACKER_BALANCE);
vm.deal(batchSender, batchSenderTargetBalance);
vm.deal(l2OutputProposer, l2OutputProposerTargetBalance);
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);
vm.expectEmit(true, true, true, true, address(balanceTracker));
emit ProcessedFunds(batchSender, false, ZERO_VALUE, ZERO_VALUE);
vm.expectEmit(true, true, true, true, address(balanceTracker));
Expand All @@ -232,7 +233,7 @@ contract BalanceTrackerTest is CommonTest {
systemAddresses.push(payable(address(uint160(i + 100))));
targetBalances.push(l2OutputProposerTargetBalance);
}
balanceTracker.initialize(systemAddresses, targetBalances);
balanceTracker.initialize(systemAddresses, targetBalances, VERSION);

balanceTracker.processFees();

Expand Down