Implement Gnosis withdrawals pull (#45)

* Withdrawals pull * Update maxFailedWithdrawalsToProcess variable name * Delete settings.json * Place all variables at the top * Update executeSystemWithdrawals comment * chore: fix all solhint issues (#49) * chore: remove global imports * chore: ignore _deprecatedUnused * chore: re-add _deprecatedUnused comment * chore: fix system tx inputs (#50) * chore: add system tx with right signature * fix: make signature public and fix tests * Update SBCDepositContract comment * Update SBCDepositContract.sol --------- Co-authored-by: Lion - dapplion <[email protected]> --------- Co-authored-by: Philippe Schommers <[email protected]>
gnosischain · Jul 11, 2023 · 24f9fcf · 24f9fcf
1 parent 13e1555
commit 24f9fcf
Show file tree

Hide file tree

Showing 18 changed files with 147 additions and 397 deletions.
diff --git a/contracts/SBCDepositContract.sol b/contracts/SBCDepositContract.sol
@@ -2,14 +2,15 @@
 
 pragma solidity 0.8.9;
 
-import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import "@openzeppelin/contracts/utils/introspection/IERC165.sol";
-import "./interfaces/IDepositContract.sol";
-import "./interfaces/IERC677Receiver.sol";
-import "./interfaces/IUnwrapper.sol";
-import "./interfaces/IWithdrawalContract.sol";
-import "./utils/PausableEIP1967Admin.sol";
-import "./utils/Claimable.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
+import {IDepositContract} from "./interfaces/IDepositContract.sol";
+import {IERC677Receiver} from "./interfaces/IERC677Receiver.sol";
+import {IUnwrapper} from "./interfaces/IUnwrapper.sol";
+import {IWithdrawalContract} from "./interfaces/IWithdrawalContract.sol";
+import {PausableEIP1967Admin} from "./utils/PausableEIP1967Admin.sol";
+import {Claimable} from "./utils/Claimable.sol";
 
 /**
  * @title SBCDepositContract
@@ -39,6 +40,9 @@ contract SBCDepositContract is
 
     IERC20 public immutable stake_token;
 
+    address private constant SYSTEM_WITHDRAWAL_EXECUTOR = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE;
+    mapping(address => uint256) public withdrawableAmount;
+
     constructor(address _token) {
         stake_token = IERC20(_token);
     }
@@ -230,182 +234,68 @@ 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.
-     */
-    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).
+     * @dev Claim withdrawal amount for an address
+     * @param _address Address to transfer withdrawable tokens
      */
-    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]);
         }
     }
 
     /**
      * @dev Function to be used only in the system transaction.
-     * Call to this function will revert only in three cases:
+     * Call to this function will revert only in case:
      *     - the caller is not `SYSTEM_WITHDRAWAL_EXECUTOR` or `_admin()`;
      *     - the length of `_amounts` array is not equal to the length of `_addresses` array;
-     *     - it is a reentrant access to failed withdrawals processing;
-     *     - 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.
+     * Call to this function doesn't transmit flow control to any untrusted contract, nor does any operation of unbounded gas usage.
+     * 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
+     * Note: chiado network requires this signature to sync post-shapella blocks. This function signature can only be deprecated after
+     * deprecating chiado network of full sync up to a pre-specified block.
+     * @custom:deprecatedparam _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 {
+    ) public {
         require(
             _msgSender() == SYSTEM_WITHDRAWAL_EXECUTOR || _msgSender() == _admin(),
             "This function should be called only by SYSTEM_WITHDRAWAL_EXECUTOR or _admin()"
         );
         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++;
+            withdrawableAmount[_addresses[i]] += amount;
         }
     }
 
     /**
-     * @dev Check if a block's withdrawal has been fully processed or not
-     * @param _withdrawalIndex EIP-4895 withdrawal.index property
+     * @dev Forwards compatible signature for `executeSystemWithdrawals` to support its future deprecation
+     * Clients must support and use the signature specified in the spec at:
+     * https://github.com/gnosischain/specs/blob/master/execution/withdrawals.md
      */
-    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;
-        }
-        // `failedWithdrawalIndex` are shifted by one for the above case
-        return failedWithdrawals[failedWithdrawalIndex - 1].amount == 0;
+    function executeSystemWithdrawals(uint64[] calldata _amounts, address[] calldata _addresses) external {
+        executeSystemWithdrawals(0, _amounts, _addresses);
     }
 
     /**

diff --git a/contracts/SBCDepositContractProxy.sol b/contracts/SBCDepositContractProxy.sol
@@ -2,8 +2,8 @@
 
 pragma solidity 0.8.9;
 
-import "./utils/EIP1967Proxy.sol";
-import "./SBCDepositContract.sol";
+import {EIP1967Proxy} from "./utils/EIP1967Proxy.sol";
+import {SBCDepositContract} from "./SBCDepositContract.sol";
 
 /**
  * @title SBCDepositContractProxy

diff --git a/contracts/SBCToken.sol b/contracts/SBCToken.sol
@@ -2,11 +2,12 @@
 
 pragma solidity 0.8.9;
 
-import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
-import "./utils/Claimable.sol";
-import "./utils/PausableEIP1967Admin.sol";
-import "./interfaces/IERC677.sol";
-import "./interfaces/IERC677Receiver.sol";
+import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
+import {ERC20Pausable} from "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
+import {Claimable} from "./utils/Claimable.sol";
+import {PausableEIP1967Admin} from "./utils/PausableEIP1967Admin.sol";
+import {IERC677} from "./interfaces/IERC677.sol";
+import {IERC677Receiver} from "./interfaces/IERC677Receiver.sol";
 
 /**
  * @title SBCToken

diff --git a/contracts/SBCTokenProxy.sol b/contracts/SBCTokenProxy.sol
@@ -2,8 +2,8 @@
 
 pragma solidity 0.8.9;
 
-import "./utils/EIP1967Proxy.sol";
-import "./SBCToken.sol";
+import {EIP1967Proxy} from "./utils/EIP1967Proxy.sol";
+import {SBCToken} from "./SBCToken.sol";
 
 /**
  * @title SBCTokenProxy

diff --git a/contracts/SBCWrapper.sol b/contracts/SBCWrapper.sol
@@ -2,12 +2,15 @@
 
 pragma solidity 0.8.9;
 
-import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-import "./interfaces/IUnwrapper.sol";
-import "./utils/PausableEIP1967Admin.sol";
-import "./SBCToken.sol";
-import "./SBCDepositContract.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+import {IUnwrapper} from "./interfaces/IUnwrapper.sol";
+import {IERC677Receiver} from "./interfaces/IERC677Receiver.sol";
+import {PausableEIP1967Admin} from "./utils/PausableEIP1967Admin.sol";
+import {SBCToken} from "./SBCToken.sol";
+import {SBCDepositContract} from "./SBCDepositContract.sol";
+import {Claimable} from "./utils/Claimable.sol";
 
 /**
  * @title SBCWrapper

diff --git a/contracts/SBCWrapperProxy.sol b/contracts/SBCWrapperProxy.sol
@@ -2,8 +2,10 @@
 
 pragma solidity 0.8.9;
 
-import "./utils/EIP1967Proxy.sol";
-import "./SBCWrapper.sol";
+import {EIP1967Proxy} from "./utils/EIP1967Proxy.sol";
+import {SBCWrapper} from "./SBCWrapper.sol";
+import {SBCToken} from "./SBCToken.sol";
+import {SBCDepositContract} from "./SBCDepositContract.sol";
 
 /**
  * @title SBCWrapperProxy

diff --git a/contracts/interfaces/IERC677.sol b/contracts/interfaces/IERC677.sol
@@ -2,7 +2,7 @@
 
 pragma solidity 0.8.9;
 
-import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 
 interface IERC677 is IERC20 {
     function transferAndCall(address to, uint256 amount, bytes calldata data) external;

diff --git a/contracts/interfaces/IPermittableToken.sol b/contracts/interfaces/IPermittableToken.sol
@@ -2,7 +2,7 @@
 
 pragma solidity 0.8.9;
 
-import "./IERC677.sol";
+import {IERC677} from "./IERC677.sol";
 
 interface IPermittableToken is IERC677 {
     function permit(

diff --git 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/contracts/test/SBCInit.sol b/contracts/test/SBCInit.sol
@@ -2,12 +2,12 @@
 
 pragma solidity ^0.8.9;
 
-import "../SBCDepositContractProxy.sol";
-import "../SBCToken.sol";
-import "../SBCTokenProxy.sol";
-import "../SBCWrapper.sol";
-import "../SBCWrapperProxy.sol";
-import "./UnsafeToken.sol";
+import {SBCDepositContractProxy} from "../SBCDepositContractProxy.sol";
+import {SBCToken} from "../SBCToken.sol";
+import {SBCTokenProxy} from "../SBCTokenProxy.sol";
+import {SBCWrapper} from "../SBCWrapper.sol";
+import {SBCWrapperProxy} from "../SBCWrapperProxy.sol";
+import {UnsafeToken} from "./UnsafeToken.sol";
 
 contract SBCInit {
     constructor(