diff --git a/contracts/SBCDepositContract.sol b/contracts/SBCDepositContract.sol index ae6f5cc..59a24fd 100644 --- a/contracts/SBCDepositContract.sol +++ b/contracts/SBCDepositContract.sol @@ -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; @@ -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); @@ -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]); } } @@ -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 { @@ -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; } /** diff --git a/contracts/interfaces/IWithdrawalContract.sol b/contracts/interfaces/IWithdrawalContract.sol index fb04b23..4d2c51a 100644 --- a/contracts/interfaces/IWithdrawalContract.sol +++ b/contracts/interfaces/IWithdrawalContract.sol @@ -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 - ); } diff --git a/test/deposit.js b/test/deposit.js index b4177ea..3d503e0 100644 --- a/test/deposit.js +++ b/test/deposit.js @@ -210,7 +210,7 @@ contract('SBCDepositContractProxy', (accounts) => { await contract.executeSystemWithdrawals(10, amounts, addresses, { from: accounts[0] }) }) - it('should correctly withdraw GNO, even with failed withdrawal', async () => { + it('should correctly withdraw GNO for one address', async () => { const amounts = ['0x0000000773594000'] // 32 * 10^9 const addresses = [accounts[1]] @@ -218,80 +218,33 @@ contract('SBCDepositContractProxy', (accounts) => { await stake.transfer(contract.address, depositAmount) await contract.executeSystemWithdrawals(0, amounts, addresses) - const mGNOBalanceAfterFirstWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterFirstWithdrawal).to.be.equal(depositAmount) + const claimableGNO = (await contract.withdrawableAmount(accounts[1])).toString() + expect(claimableGNO).to.be.equal(depositAmount) + await contract.claimWithdrawal(accounts[1]) + const GNOBalanceAfterWithdrawal = (await stake.balanceOf(accounts[1])).toString() + expect(GNOBalanceAfterWithdrawal).to.be.equal(depositAmount) + }) - // failed and processed by queue - await contract.executeSystemWithdrawals(0, amounts, addresses) - let numberOfFailedWithdrawals = (await contract.numberOfFailedWithdrawals()).toString() - expect(numberOfFailedWithdrawals).to.be.equal('1') - - await stake.transfer(contract.address, depositAmount) - - await contract.processFailedWithdrawalsFromPointer(5) - const mGNOBalanceAfterSecondWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterSecondWithdrawal).to.be.equal(web3.utils.toWei('2')) - let failedWithdrawalsPointer = (await contract.failedWithdrawalsPointer()).toString() - expect(failedWithdrawalsPointer).to.be.equal('1') - await contract.processFailedWithdrawal(0, 0).should.be.rejected - - - // failed and processed by queue in executeSystemWithdrawals - await contract.executeSystemWithdrawals(0, amounts, addresses) - numberOfFailedWithdrawals = (await contract.numberOfFailedWithdrawals()).toString() - expect(numberOfFailedWithdrawals).to.be.equal('2') - - await stake.transfer(contract.address, depositAmount) - - await contract.executeSystemWithdrawals(2, [], []) - const mGNOBalanceAfterThirdWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterThirdWithdrawal).to.be.equal(web3.utils.toWei('3')) - failedWithdrawalsPointer = (await contract.failedWithdrawalsPointer()).toString() - expect(failedWithdrawalsPointer).to.be.equal('2') - await contract.processFailedWithdrawal(1, 0).should.be.rejected - - - // failed and processed manually - await contract.executeSystemWithdrawals(0, amounts, addresses) - numberOfFailedWithdrawals = (await contract.numberOfFailedWithdrawals()).toString() - expect(numberOfFailedWithdrawals).to.be.equal('3') - - await stake.transfer(contract.address, depositAmount) - - await contract.processFailedWithdrawal(2, 0) - await contract.processFailedWithdrawal(2, 0).should.be.rejected - - let mGNOBalanceAfterFourthWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterFourthWithdrawal).to.be.equal(web3.utils.toWei('4')) - failedWithdrawalsPointer = (await contract.failedWithdrawalsPointer()).toString() - expect(failedWithdrawalsPointer).to.be.equal('2') - await contract.processFailedWithdrawalsFromPointer(5) - mGNOBalanceAfterFourthWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterFourthWithdrawal).to.be.equal(web3.utils.toWei('4')) - failedWithdrawalsPointer = (await contract.failedWithdrawalsPointer()).toString() - expect(failedWithdrawalsPointer).to.be.equal('3') + it('should correctly withdraw GNO for multiple addresses', async () => { + const addressCount = 4; + const amounts = Array.from({length: addressCount}, () => '0x0000000773594000') // 32 * 10^9 + const addresses = accounts.slice(1, 1 + addressCount) + // simple withdrawal + await stake.transfer(contract.address, web3.utils.toWei(String(addressCount))) - // failed and processed partially manually await contract.executeSystemWithdrawals(0, amounts, addresses) - numberOfFailedWithdrawals = (await contract.numberOfFailedWithdrawals()).toString() - expect(numberOfFailedWithdrawals).to.be.equal('4') - - await stake.transfer(contract.address, depositAmount) - - await contract.processFailedWithdrawal(3, halfDepositAmount, { from : addresses[0] }) - - let mGNOBalanceAfterFifthWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterFifthWithdrawal).to.be.equal(web3.utils.toWei('4.5')) - - await contract.processFailedWithdrawal(3, 0) - - mGNOBalanceAfterFifthWithdrawal = (await stake.balanceOf(accounts[1])).toString() - expect(mGNOBalanceAfterFifthWithdrawal).to.be.equal(web3.utils.toWei('5')) + for (let i = 0; i < addressCount; i++) { + const claimableGNO = (await contract.withdrawableAmount(addresses[i])).toString() + expect(claimableGNO).to.be.equal(depositAmount) + } - await contract.processFailedWithdrawal(3, 0).should.be.rejected - await contract.processFailedWithdrawal(4, 0).should.be.rejected + await contract.claimWithdrawals(addresses) + for (let i = 0; i < addressCount; i++) { + const GNOBalanceAfterWithdrawal = (await stake.balanceOf(addresses[i])).toString() + expect(GNOBalanceAfterWithdrawal).to.be.equal(depositAmount) + } }) it('should correctly withdraw mGNO with amount = 0', async () => { @@ -301,7 +254,7 @@ contract('SBCDepositContractProxy', (accounts) => { // simple withdrawal await token.transfer(contract.address, thirtyTwoEther) - assertSuccessfulWithdrawal(await contract.executeSystemWithdrawals(0, amounts, addresses)) + await contract.executeSystemWithdrawals(0, amounts, addresses) const mGNOBalanceAfterWithdrawal = (await token.balanceOf(accounts[1])).toString() expect(mGNOBalanceAfterWithdrawal).to.be.equal(web3.utils.toWei('0')) }) @@ -313,51 +266,11 @@ contract('SBCDepositContractProxy', (accounts) => { // simple withdrawal await token.transfer(contract.address, thirtyTwoEther) - assertSuccessfulWithdrawal(await contract.executeSystemWithdrawals(0, amounts, addresses)) + await contract.executeSystemWithdrawals(0, amounts, addresses) const mGNOBalanceAfterFirstWithdrawal = (await token.balanceOf(zeroAddress)).toString() expect(mGNOBalanceAfterFirstWithdrawal).to.be.equal(zeroEther) }) - it("isWithdrawalProcessed first withdrawal successful", async () => { - // No withdrawals yet, none should be processed - // Withdrawals state = [] - await assertWithdrawalsState([]); - - await executeSuccessfulWithdrawal(); - // Withdrawal state = [success] - await assertWithdrawalsState([true]); - - await executeFailedWithdrawal(); - // Withdrawal state = [success, failed] - await assertWithdrawalsState([true, false]); - - await reexecuteFailedWithdrawal(); - // Withdrawal state = [success, success] - await assertWithdrawalsState([true, true]); - }); - - it("isWithdrawalProcessed first withdrawal failed", async () => { - // No withdrawals yet, none should be processed - // Withdrawals state = [] - await assertWithdrawalsState([]); - - await executeFailedWithdrawal(); - // Withdrawal state = [failed] - await assertWithdrawalsState([false]); - - await executeFailedWithdrawal(); - // Withdrawal state = [failed, failed] - await assertWithdrawalsState([false, false]); - - await reexecuteFailedWithdrawal(); - // Withdrawal state = [failed, success] - await assertWithdrawalsState([true, false]); - - await reexecuteFailedWithdrawal(); - // Withdrawal state = [success, success] - await assertWithdrawalsState([true, true]); - }); - it('should claim tokens', async () => { const otherToken = await IERC677.new() await stake.transfer(contract.address, 1) @@ -388,40 +301,4 @@ contract('SBCDepositContractProxy', (accounts) => { await contract.unwrapTokens(wrapper.address, token.address) expect((await stake.balanceOf(contract.address)).toString()).to.be.equal(web3.utils.toWei('42')) }) - - async function executeSuccessfulWithdrawal() { - const amounts = ["0x0000000773594000"] // 32 * 10^9 - const addresses = [accounts[1]] - await stake.transfer(contract.address, depositAmount) - await contract.executeSystemWithdrawals(0, amounts, addresses) - } - - async function executeFailedWithdrawal() { - const amounts = ["0x0000000773594000"] // 32 * 10^9 - const addresses = [accounts[1]] - await contract.executeSystemWithdrawals(0, amounts, addresses) - } - - async function reexecuteFailedWithdrawal() { - await stake.transfer(contract.address, depositAmount) - await contract.executeSystemWithdrawals(1, [], []) - } - - // Call with bool array where true = successful withdrawal - async function assertWithdrawalsState(withdrawalState) { - for (let i = 0; i < withdrawalState.length; i++) { - expect(await contract.isWithdrawalProcessed(i)).equal( - withdrawalState[i], - `wrong isWithdrawalProcessed(${i}), withdrawalState ${JSON.stringify(withdrawalState)}` - ) - } - // Should revert with out-of-bounds on the next withdrawalState - await contract.isWithdrawalProcessed(withdrawalState.length).should.be.rejected - } }) - -function assertSuccessfulWithdrawal(tx) { - const withdrawEvent = tx.logs.find(log => log.event.startsWith("Withdrawal")) - if (!withdrawEvent) throw Error('tx has no Withdraw* events') - expect(withdrawEvent.event).equal('WithdrawalExecuted') -} \ No newline at end of file