Skip to content

fix/deprecated slots gap #47

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

Closed
4rgon4ut wants to merge 5 commits into from
Closed

fix/deprecated slots gap #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
961df1b
Withdrawals pull
dapplion Jun 7, 2023
77a7756
Update maxFailedWithdrawalsToProcess variable name
dapplion Jun 15, 2023
0afa697
Delete settings.json
dapplion Jun 15, 2023
8fe75d2
Place all variables at the top
dapplion Jun 20, 2023
006264f
fix: add deprecated slots gap
4rgon4ut Jun 22, 2023
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
167 changes: 26 additions & 141 deletions contracts/SBCDepositContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ contract SBCDepositContract is
uint256 private constant DEPOSIT_CONTRACT_TREE_DEPTH = 32;
// NOTE: this also ensures `deposit_count` will fit into 64-bits
uint256 private constant MAX_DEPOSIT_COUNT = 2 ** DEPOSIT_CONTRACT_TREE_DEPTH - 1;
address private constant SYSTEM_WITHDRAWAL_EXECUTOR = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE;

IERC20 public immutable stake_token;

bytes32[DEPOSIT_CONTRACT_TREE_DEPTH] private zero_hashes;

Expand All @@ -37,7 +40,11 @@ contract SBCDepositContract is

mapping(bytes => bytes32) public validator_withdrawal_credentials;

IERC20 public immutable stake_token;
// This gap contains deprecated variables storage slots (67-71 inclusively).
// New state variables MUST be added AFTER this gap to follow historical storage layout.
uint256[5] private _deprecated_slots_gap;

mapping(address => uint256) public withdrawableAmount;

constructor(address _token) {
stake_token = IERC20(_token);
Expand Down Expand Up @@ -230,114 +237,25 @@ contract SBCDepositContract is

/*** Withdrawal part ***/

address private constant SYSTEM_WITHDRAWAL_EXECUTOR = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE;

uint256 private constant DEFAULT_GAS_PER_WITHDRAWAL = 300000;

/**
* @dev Function to be used to process a withdrawal.
* Actually it is an internal function, only this contract can call it.
* This is done in order to roll back all changes in case of revert.
* @param _amount Amount to be withdrawn.
* @param _receiver Receiver of the withdrawal.
*/
function processWithdrawalInternal(uint256 _amount, address _receiver) external {
require(msg.sender == address(this), "Should be used only as an internal call");
stake_token.safeTransfer(_receiver, _amount);
}

/**
* @dev Internal function to be used to process a withdrawal.
* Uses processWithdrawalInternal under the hood.
* Call to this function will revert only if it ran out of gas.
* @param _amount Amount to be withdrawn.
* @param _receiver Receiver of the withdrawal.
* @return success An indicator of whether the withdrawal was successful or not.
* @dev Claim withdrawal amount for an address
* @param _address Address to transfer withdrawable tokens
*/
function _processWithdrawal(uint256 _amount, address _receiver, uint256 gasLimit) internal returns (bool success) {
// Skip withdrawal that burns tokens to avoid a revert condition
// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/dad73159df3d3053c72b5e430fa8164330f18068/contracts/token/ERC20/ERC20.sol#L278
if (_receiver == address(0)) {
return true;
}

try this.processWithdrawalInternal{gas: gasLimit}(_amount, _receiver) {
return true;
} catch {
return false;
}
}

struct FailedWithdrawalRecord {
uint256 amount;
address receiver;
uint64 withdrawalIndex;
}
mapping(uint256 => FailedWithdrawalRecord) public failedWithdrawals;
mapping(uint64 => uint256) public failedWithdrawalIndexByWithdrawalIndex;
uint256 public numberOfFailedWithdrawals;
uint64 public nextWithdrawalIndex;

/**
* @dev Function to be used to process a failed withdrawal (possibly partially).
* @param _failedWithdrawalId Id of a failed withdrawal.
* @param _amountToProceed Amount of token to withdraw (for the case it is impossible to withdraw the full amount)
* (available only for the receiver, will be ignored if other account tries to process the withdrawal).
*/
function processFailedWithdrawal(uint256 _failedWithdrawalId, uint256 _amountToProceed) external whenNotPaused {
require(_failedWithdrawalId < numberOfFailedWithdrawals, "Failed withdrawal do not exist");

FailedWithdrawalRecord storage failedWithdrawalRecord = failedWithdrawals[_failedWithdrawalId];
require(failedWithdrawalRecord.amount > 0, "Failed withdrawal already processed");

uint256 amountToProceed = failedWithdrawalRecord.amount;
if (_msgSender() == failedWithdrawalRecord.receiver) {
if (_amountToProceed != 0) {
require(_amountToProceed <= failedWithdrawalRecord.amount, "Invalid amount of tokens");
amountToProceed = _amountToProceed;
}
function claimWithdrawal(address _address) public {
uint256 amount = withdrawableAmount[_address];
if (amount > 0) {
withdrawableAmount[_address] = 0;
stake_token.safeTransfer(_address, amount);
}

failedWithdrawalRecord.amount -= amountToProceed;
bool success = _processWithdrawal(amountToProceed, failedWithdrawalRecord.receiver, gasleft());
require(success, "Withdrawal processing failed");
emit FailedWithdrawalProcessed(_failedWithdrawalId, amountToProceed, failedWithdrawalRecord.receiver);
}

uint256 public failedWithdrawalsPointer;

/**
* @dev Function to be used to process failed withdrawals.
* Call to this function will revert only if it ran out of gas or it is a reentrant access to failed withdrawals processing.
* Call to this function doesn't transmit flow control to any untrusted contract and uses a constant gas limit for each withdrawal,
* so using constant gas limit and constant max number of withdrawals for calls of this function is ok.
* @param _maxNumberOfFailedWithdrawalsToProcess Maximum number of failed withdrawals to be processed.
* @dev Claim withdrawal amounts for an array of addresses
* @param _addresses Addresses to transfer withdrawable tokens
*/
function processFailedWithdrawalsFromPointer(uint256 _maxNumberOfFailedWithdrawalsToProcess) public {
for (uint256 i = 0; i < _maxNumberOfFailedWithdrawalsToProcess; ++i) {
if (failedWithdrawalsPointer == numberOfFailedWithdrawals) {
break;
}

FailedWithdrawalRecord storage failedWithdrawalRecord = failedWithdrawals[failedWithdrawalsPointer];
if (failedWithdrawalRecord.amount > 0) {
// Reentrancy guard
uint256 amount = failedWithdrawalRecord.amount;
failedWithdrawalRecord.amount = 0;

// Execute the withdrawal
bool success = _processWithdrawal(amount, failedWithdrawalRecord.receiver, DEFAULT_GAS_PER_WITHDRAWAL);

if (!success) {
// Reset the record amount for the reentrancy guard
failedWithdrawalRecord.amount = amount;
break;
}

emit FailedWithdrawalProcessed(failedWithdrawalsPointer, amount, failedWithdrawalRecord.receiver);
}

++failedWithdrawalsPointer;
function claimWithdrawals(address[] calldata _addresses) external {
for (uint256 i = 0; i < _addresses.length; ++i) {
claimWithdrawal(_addresses[i]);
}
}

Expand All @@ -350,12 +268,15 @@ contract SBCDepositContract is
* - the call ran out of gas.
* Call to this function doesn't transmit flow control to any untrusted contract and uses a constant gas limit for each withdrawal,
* so using constant gas limit and constant number of withdrawals (including failed withdrawals) for calls of this function is ok.
* @param _maxNumberOfFailedWithdrawalsToProcess Maximum number of failed withdrawals to be processed.
* NOTE: This function signature is hardcoded in the Gnosis execution layer clients. Changing this signature without updating the
* clients will cause block verification of any post-shangai block to fail. The function signature cannonical spec is here
* https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md
* @param _deprecatedUnused Previously `maxFailedWithdrawalsToProcess` currently deprecated and ignored
* @param _amounts Array of amounts to be withdrawn.
* @param _addresses Array of addresses that should receive the corresponding amount of tokens.
*/
function executeSystemWithdrawals(
uint256 _maxNumberOfFailedWithdrawalsToProcess,
uint256 _deprecatedUnused,
uint64[] calldata _amounts,
address[] calldata _addresses
) external {
Expand All @@ -365,47 +286,11 @@ contract SBCDepositContract is
);
assert(_amounts.length == _addresses.length);

processFailedWithdrawalsFromPointer(_maxNumberOfFailedWithdrawalsToProcess);

for (uint256 i = 0; i < _amounts.length; ++i) {
// Divide stake amount by 32 (1 GNO for validating instead of the 32 ETH expected)
uint256 amount = (uint256(_amounts[i]) * 1 gwei) / 32;
bool success = _processWithdrawal(amount, _addresses[i], DEFAULT_GAS_PER_WITHDRAWAL);

if (success) {
emit WithdrawalExecuted(amount, _addresses[i]);
} else {
failedWithdrawals[numberOfFailedWithdrawals] = FailedWithdrawalRecord({
amount: amount,
receiver: _addresses[i],
withdrawalIndex: nextWithdrawalIndex
});
// Shift `failedWithdrawalIndex` by one to allow `isWithdrawalProcessed()`
// to detect successful withdrawals
failedWithdrawalIndexByWithdrawalIndex[nextWithdrawalIndex] = numberOfFailedWithdrawals + 1;
emit WithdrawalFailed(numberOfFailedWithdrawals, amount, _addresses[i]);
++numberOfFailedWithdrawals;
}

// First withdrawal is index 0
nextWithdrawalIndex++;
}
}

/**
* @dev Check if a block's withdrawal has been fully processed or not
* @param _withdrawalIndex EIP-4895 withdrawal.index property
*/
function isWithdrawalProcessed(uint64 _withdrawalIndex) external view returns (bool) {
require(_withdrawalIndex < nextWithdrawalIndex, "withdrawal_index out-of-bounds");
// Only failed withdrawals are registered into failedWithdrawalByIndex, so successful withdrawals
// `_withdrawalIndex` return `failedWithdrawalIndex` 0.
uint256 failedWithdrawalIndex = failedWithdrawalIndexByWithdrawalIndex[_withdrawalIndex];
if (failedWithdrawalIndex == 0) {
return true;
withdrawableAmount[_addresses[i]] += amount;
}
// `failedWithdrawalIndex` are shifted by one for the above case
return failedWithdrawals[failedWithdrawalIndex - 1].amount == 0;
}

/**
Expand Down
30 changes: 0 additions & 30 deletions contracts/interfaces/IWithdrawalContract.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,34 +21,4 @@ interface IWithdrawalContract {
uint64[] calldata _amounts,
address[] calldata _addresses
) external;

/// @notice Executed withdrawal event.
event WithdrawalExecuted(uint256 _amount, address indexed _address);

/// @notice Failed withdrawal event.
event WithdrawalFailed(uint256 indexed _failedWithdrawalId, uint256 _amount, address indexed _address);

/**
* @dev Function to be used to process failed withdrawals.
* Call to this function will revert only if it ran out of gas or it is a reentrant access to failed withdrawals processing.
* Call to this function doesn't transmit flow control to any untrusted contract and uses a constant gas limit for each withdrawal,
* so using constant gas limit and constant max number of withdrawals for calls of this function is ok.
* @param _maxNumberOfFailedWithdrawalsToProcess Maximum number of failed withdrawals to be processed.
*/
function processFailedWithdrawalsFromPointer(uint256 _maxNumberOfFailedWithdrawalsToProcess) external;

/**
* @dev Function to be used to process a failed withdrawal (possibly partially).
* @param _failedWithdrawalId Id of a failed withdrawal.
* @param _amountToProceed Amount of token to withdraw (for the case it is impossible to withdraw the full amount)
* (available only for the receiver, will be ignored if other account tries to process the withdrawal).
*/
function processFailedWithdrawal(uint256 _failedWithdrawalId, uint256 _amountToProceed) external;

/// @notice Processed (possibly partially) failed withdrawal event.
event FailedWithdrawalProcessed(
uint256 indexed _failedWithdrawalId,
uint256 _processedAmount,
address indexed _address
);
}
Loading