Merge pull request #751 from lidofinance/fix/shapella-statemind-findi…

…ngs-round2

Statemind minor fixes round 2

contracts/0.4.24/Lido.sol

@@ -282,12 +282,12 @@ contract Lido is Versioned, StETHPermit, AragonApp {
         LIDO_LOCATOR_POSITION.setStorageAddress(_lidoLocator);
         _initializeEIP712StETH(_eip712StETH);
 
-        // set unlimited allowance for burner from withdrawal queue
+        // set infinite allowance for burner from withdrawal queue
         // to burn finalized requests' shares
         _approve(
             ILidoLocator(_lidoLocator).withdrawalQueue(),
             ILidoLocator(_lidoLocator).burner(),
-           ~uint256(0)
+            INFINITE_ALLOWANCE
         );
 
         emit LidoLocatorSet(_lidoLocator);

contracts/0.4.24/StETH.sol

@@ -51,6 +51,7 @@ contract StETH is IERC20, Pausable {
     using UnstructuredStorage for bytes32;
 
     address constant internal INITIAL_TOKEN_HOLDER = 0xdead;
+    uint256 constant internal INFINITE_ALLOWANCE = ~uint256(0);
 
     /**
      * @dev StETH balances are dynamic and are calculated based on the accounts' shares
@@ -234,11 +235,8 @@ contract StETH is IERC20, Pausable {
      * @dev The `_amount` argument is the amount of tokens, not shares.
      */
     function transferFrom(address _sender, address _recipient, uint256 _amount) external returns (bool) {
-        uint256 currentAllowance = allowances[_sender][msg.sender];
-        require(currentAllowance >= _amount, "ALLOWANCE_EXCEEDED");
-
+        _spendAllowance(_sender, msg.sender, _amount);
         _transfer(_sender, _recipient, _amount);
-        _approve(_sender, msg.sender, currentAllowance.sub(_amount));
         return true;
     }
 
@@ -247,7 +245,7 @@ contract StETH is IERC20, Pausable {
      *
      * This is an alternative to `approve` that can be used as a mitigation for
      * problems described in:
-     * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L42
+     * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/b709eae01d1da91902d06ace340df6b324e6f049/contracts/token/ERC20/IERC20.sol#L57
      * Emits an `Approval` event indicating the updated allowance.
      *
      * Requirements:
@@ -264,7 +262,7 @@ contract StETH is IERC20, Pausable {
      *
      * This is an alternative to `approve` that can be used as a mitigation for
      * problems described in:
-     * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol#L42
+     * https://github.com/OpenZeppelin/openzeppelin-contracts/blob/b709eae01d1da91902d06ace340df6b324e6f049/contracts/token/ERC20/IERC20.sol#L57
      * Emits an `Approval` event indicating the updated allowance.
      *
      * Requirements:
@@ -355,12 +353,9 @@ contract StETH is IERC20, Pausable {
     function transferSharesFrom(
         address _sender, address _recipient, uint256 _sharesAmount
     ) external returns (uint256) {
-        uint256 currentAllowance = allowances[_sender][msg.sender];
         uint256 tokensAmount = getPooledEthByShares(_sharesAmount);
-        require(currentAllowance >= tokensAmount, "ALLOWANCE_EXCEEDED");
-
+        _spendAllowance(_sender, msg.sender, tokensAmount);
         _transferShares(_sender, _recipient, _sharesAmount);
-        _approve(_sender, msg.sender, currentAllowance.sub(tokensAmount));
         _emitTransferEvents(_sender, _recipient, tokensAmount, _sharesAmount);
         return tokensAmount;
     }
@@ -403,6 +398,22 @@ contract StETH is IERC20, Pausable {
         emit Approval(_owner, _spender, _amount);
     }
 
+    /**
+     * @dev Updates `owner` s allowance for `spender` based on spent `amount`.
+     *
+     * Does not update the allowance amount in case of infinite allowance.
+     * Revert if not enough allowance is available.
+     *
+     * Might emit an {Approval} event.
+     */
+    function _spendAllowance(address _owner, address _spender, uint256 _amount) internal {
+        uint256 currentAllowance = allowances[_owner][_spender];
+        if (currentAllowance != INFINITE_ALLOWANCE) {
+            require(currentAllowance >= _amount, "ALLOWANCE_EXCEEDED");
+            _approve(_owner, _spender, currentAllowance - _amount);
+        }
+    }
+
     /**
      * @return the total amount of shares in existence.
      */

contracts/0.8.9/Burner.sol

@@ -61,7 +61,6 @@ contract Burner is IBurner, AccessControlEnumerable {
     error ZeroAddress(string field);
 
     bytes32 public constant REQUEST_BURN_MY_STETH_ROLE = keccak256("REQUEST_BURN_MY_STETH_ROLE");
-    bytes32 public constant RECOVER_ASSETS_ROLE = keccak256("RECOVER_ASSETS_ROLE");
     bytes32 public constant REQUEST_BURN_SHARES_ROLE = keccak256("REQUEST_BURN_SHARES_ROLE");
 
     uint256 private coverSharesBurnRequested;
@@ -223,7 +222,7 @@ contract Burner is IBurner, AccessControlEnumerable {
       * but not marked for burning) to the Lido treasury address set upon the
       * contract construction.
       */
-    function recoverExcessStETH() external onlyRole(RECOVER_ASSETS_ROLE) {
+    function recoverExcessStETH() external {
         uint256 excessStETH = getExcessStETH();
 
         if (excessStETH > 0) {
@@ -249,7 +248,7 @@ contract Burner is IBurner, AccessControlEnumerable {
       * @param _token an ERC20-compatible token
       * @param _amount token amount
       */
-    function recoverERC20(address _token, uint256 _amount) external onlyRole(RECOVER_ASSETS_ROLE) {
+    function recoverERC20(address _token, uint256 _amount) external {
         if (_amount == 0) revert ZeroRecoveryAmount();
         if (_token == STETH) revert StETHRecoveryWrongFunc();
 
@@ -265,7 +264,7 @@ contract Burner is IBurner, AccessControlEnumerable {
       * @param _token an ERC721-compatible token
       * @param _tokenId minted token id
       */
-    function recoverERC721(address _token, uint256 _tokenId) external onlyRole(RECOVER_ASSETS_ROLE) {
+    function recoverERC721(address _token, uint256 _tokenId) external {
         if (_token == STETH) revert StETHRecoveryWrongFunc();
 
         emit ERC721Recovered(msg.sender, _token, _tokenId);

contracts/0.8.9/WithdrawalQueue.sol

@@ -71,6 +71,7 @@ abstract contract WithdrawalQueue is AccessControlEnumerable, PausableUntil, Wit
     error InvalidReportTimestamp();
     error RequestIdsNotSorted();
     error ZeroRecipient();
+    error ArraysLengthMismatch(uint256 _firstArrayLength, uint256 _secondArrayLength);
 
     /// @param _wstETH address of WstETH contract
     constructor(IWstETH _wstETH) {
@@ -236,13 +237,17 @@ abstract contract WithdrawalQueue is AccessControlEnumerable, PausableUntil, Wit
     /// @param _recipient address where claimed ether will be sent to
     /// @dev
     ///  Reverts if recipient is equal to zero
+    ///  Reverts if requestIds and hints arrays length differs
     ///  Reverts if any requestId or hint in arguments are not valid
     ///  Reverts if any request is not finalized or already claimed
     ///  Reverts if msg sender is not an owner of the requests
     function claimWithdrawalsTo(uint256[] calldata _requestIds, uint256[] calldata _hints, address _recipient)
         external
     {
         if (_recipient == address(0)) revert ZeroRecipient();
+        if (_requestIds.length != _hints.length) {
+            revert ArraysLengthMismatch(_requestIds.length, _hints.length);
+        }
 
         for (uint256 i = 0; i < _requestIds.length; ++i) {
             _claim(_requestIds[i], _hints[i], _recipient);
@@ -254,10 +259,15 @@ abstract contract WithdrawalQueue is AccessControlEnumerable, PausableUntil, Wit
     /// @param _requestIds array of request ids to claim
     /// @param _hints checkpoint hint for each id. Can be obtained with `findCheckpointHints()`
     /// @dev
+    ///  Reverts if requestIds and hints arrays length differs
     ///  Reverts if any requestId or hint in arguments are not valid
     ///  Reverts if any request is not finalized or already claimed
     ///  Reverts if msg sender is not an owner of the requests
     function claimWithdrawals(uint256[] calldata _requestIds, uint256[] calldata _hints) external {
+        if (_requestIds.length != _hints.length) {
+            revert ArraysLengthMismatch(_requestIds.length, _hints.length);
+        }
+
         for (uint256 i = 0; i < _requestIds.length; ++i) {
             _claim(_requestIds[i], _hints[i], msg.sender);
             _emitTransfer(msg.sender, address(0), _requestIds[i]);

contracts/0.8.9/WithdrawalQueueBase.sol

@@ -474,7 +474,7 @@ abstract contract WithdrawalQueueBase {
         // (issue: https://github.com/lidofinance/lido-dao/issues/442 )
         // some dust (1-2 wei per request) will be accumulated upon claiming
         _setLockedEtherAmount(getLockedEtherAmount() - ethWithDiscount);
-        _sendValue(payable(_recipient), ethWithDiscount);
+        _sendValue(_recipient, ethWithDiscount);
 
         emit WithdrawalClaimed(_requestId, msg.sender, _recipient, ethWithDiscount);
     }

contracts/0.8.9/oracle/BaseOracle.sol

@@ -50,6 +50,7 @@ abstract contract BaseOracle is IReportAsyncProcessor, AccessControlEnumerable,
     error UnexpectedConsensusVersion(uint256 expectedVersion, uint256 receivedVersion);
     error HashCannotBeZero();
     error UnexpectedDataHash(bytes32 consensusHash, bytes32 receivedHash);
+    error SecondsPerSlotCannotBeZero();
 
     event ConsensusHashContractSet(address indexed addr, address indexed prevAddr);
     event ConsensusVersionSet(uint256 indexed version, uint256 indexed prevVersion);
@@ -100,6 +101,7 @@ abstract contract BaseOracle is IReportAsyncProcessor, AccessControlEnumerable,
     ///
 
     constructor(uint256 secondsPerSlot, uint256 genesisTime) {
+        if (secondsPerSlot == 0) revert SecondsPerSlotCannotBeZero();
         SECONDS_PER_SLOT = secondsPerSlot;
         GENESIS_TIME = genesisTime;
     }

contracts/common/lib/ECDSA.sol

@@ -35,7 +35,6 @@ library ECDSA {
         // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
         // these malleable signatures as well.
         require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0, "ECDSA: invalid signature 's' value");
-        require(v == 27 || v == 28, "ECDSA: invalid signature 'v' value");
 
         // If the signature is valid (and not malleable), return the signer address
         address signer = ecrecover(hash, v, r, s);

test/0.4.24/steth.test.js

@@ -2,9 +2,9 @@ const { artifacts, contract, ethers } = require('hardhat')
 const { assert } = require('../helpers/assert')
 
 const { bn } = require('@aragon/contract-helpers-test')
-const { tokens, ETH } = require('./../helpers/utils')
+const { tokens, ETH, shares } = require('./../helpers/utils')
 const { EvmSnapshot } = require('../helpers/blockchain')
-const { INITIAL_HOLDER, ZERO_ADDRESS } = require('../helpers/constants')
+const { INITIAL_HOLDER, ZERO_ADDRESS, MAX_UINT256 } = require('../helpers/constants')
 
 const StETHMock = artifacts.require('StETHMock')
 
@@ -187,7 +187,7 @@ contract('StETH', ([_, __, user1, user2, user3, nobody]) => {
         it('reverts when sender is zero address', async () => {
           await assert.reverts(
             stEth.transferFrom(ZERO_ADDRESS, user3, tokens(0), { from: user2 }),
-            'TRANSFER_FROM_ZERO_ADDR'
+            'APPROVE_FROM_ZERO_ADDR'
           )
         })
 
@@ -213,7 +213,27 @@ contract('StETH', ([_, __, user1, user2, user3, nobody]) => {
             to: user3,
             sharesValue: sharesAmount,
           })
-          assert.equals(await stEth.allowance(user2, user1), bn(0))
+          assert.equals(await stEth.allowance(user1, user2), bn(0))
+          assert.equals(await stEth.balanceOf(user1), tokens(49))
+          assert.equals(await stEth.balanceOf(user3), tokens(50))
+        })
+
+        it("doesn't spent allowance if it was set to MAX_UINT256", async () => {
+          await stEth.approve(user2, MAX_UINT256, { from: user1 })
+          assert.equals(await stEth.allowance(user1, user2), bn(MAX_UINT256))
+          const amount = tokens(50)
+          const receipt = await stEth.transferFrom(user1, user3, amount, { from: user2 })
+          assert.emitsNumberOfEvents(receipt, 'Transfer', 1)
+          assert.emitsNumberOfEvents(receipt, 'TransferShares', 1)
+          assert.emitsNumberOfEvents(receipt, 'Approval', 0)
+          assert.emits(receipt, 'Transfer', { from: user1, to: user3, value: amount })
+          const sharesAmount = await stEth.getSharesByPooledEth(amount)
+          assert.emits(receipt, 'TransferShares', {
+            from: user1,
+            to: user3,
+            sharesValue: sharesAmount,
+          })
+          assert.equals(await stEth.allowance(user1, user2), bn(MAX_UINT256))
           assert.equals(await stEth.balanceOf(user1), tokens(49))
           assert.equals(await stEth.balanceOf(user3), tokens(50))
         })
@@ -660,6 +680,26 @@ contract('StETH', ([_, __, user1, user2, user3, nobody]) => {
         assert.equals(await stEth.balanceOf(user1), tokens(0))
         assert.equals(await stEth.balanceOf(nobody), '118800000000000000000')
       })
+
+      it("transferSharesFrom doesn't spent allowance if it was set to MAX_UINT256", async () => {
+        await stEth.approve(user2, MAX_UINT256, { from: user1 })
+        assert.equals(await stEth.allowance(user1, user2), bn(MAX_UINT256))
+        const sharesAmount = shares(50)
+        const tokensAmount = await stEth.getPooledEthByShares(sharesAmount)
+        const receipt = await stEth.transferSharesFrom(user1, user3, sharesAmount, { from: user2 })
+        assert.emitsNumberOfEvents(receipt, 'Transfer', 1)
+        assert.emitsNumberOfEvents(receipt, 'TransferShares', 1)
+        assert.emitsNumberOfEvents(receipt, 'Approval', 0)
+        assert.emits(receipt, 'Transfer', { from: user1, to: user3, value: tokensAmount })
+        assert.emits(receipt, 'TransferShares', {
+          from: user1,
+          to: user3,
+          sharesValue: sharesAmount,
+        })
+        assert.equals(await stEth.allowance(user1, user2), bn(MAX_UINT256))
+        assert.equals(await stEth.balanceOf(user1), tokens(49))
+        assert.equals(await stEth.balanceOf(user3), tokens(50))
+      })
     })
   })
 })