From 3eec54b20b8c64d385f2effd62d833188b28d8c9 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 12 Apr 2023 15:10:07 +0400 Subject: [PATCH 1/7] Update github links in StETH comments --- contracts/0.4.24/StETH.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/0.4.24/StETH.sol b/contracts/0.4.24/StETH.sol index 86ec56d53..681fa0aa6 100644 --- a/contracts/0.4.24/StETH.sol +++ b/contracts/0.4.24/StETH.sol @@ -247,7 +247,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 +264,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: From 7a40ae999377899a3ddf2e277bd6bf5fddead136 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 12 Apr 2023 16:35:57 +0400 Subject: [PATCH 2/7] Remove onlyRole modifier from Burner token recovery methods --- contracts/0.8.9/Burner.sol | 7 ++- test/0.8.9/burner.test.js | 102 ++++++------------------------------- test/helpers/factories.js | 4 +- 3 files changed, 20 insertions(+), 93 deletions(-) diff --git a/contracts/0.8.9/Burner.sol b/contracts/0.8.9/Burner.sol index 7d9e44e7b..696a2eb2d 100644 --- a/contracts/0.8.9/Burner.sol +++ b/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); diff --git a/test/0.8.9/burner.test.js b/test/0.8.9/burner.test.js index 88dcc6f6a..4e3957cc5 100644 --- a/test/0.8.9/burner.test.js +++ b/test/0.8.9/burner.test.js @@ -66,44 +66,6 @@ contract('Burner', ([deployer, _, anotherAccount]) => { await burner.requestBurnMyStETHForCover(StETH(1), { from: anotherAccount }) }) - it(`RECOVER_ASSETS_ROLE works`, async () => { - const nft1 = bn(666) - const totalERC20Supply = bn(1000000) - const mockERC20Token = await ERC20OZMock.new(totalERC20Supply, { from: deployer }) - const mockNFT = await ERC721OZMock.new({ from: deployer }) - await mockNFT.mintToken(nft1, { from: deployer }) - await web3.eth.sendTransaction({ from: anotherAccount, to: lido.address, value: ETH(2) }) - - await mockERC20Token.transfer(burner.address, bn(600000), { from: deployer }) - await mockNFT.transferFrom(deployer, burner.address, nft1, { from: deployer }) - await lido.transfer(burner.address, StETH(1), { from: anotherAccount }) - - assert.isFalse(await burner.hasRole(await burner.RECOVER_ASSETS_ROLE(), anotherAccount)) - - await assert.revertsOZAccessControl( - burner.recoverERC20(mockERC20Token.address, bn(600000), { from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - await assert.revertsOZAccessControl( - burner.recoverERC721(mockNFT.address, nft1, { from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - await assert.revertsOZAccessControl( - burner.recoverExcessStETH({ from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - - await burner.grantRole(await burner.RECOVER_ASSETS_ROLE(), anotherAccount, { from: appManager }) - assert.isTrue(await burner.hasRole(await burner.RECOVER_ASSETS_ROLE(), anotherAccount)) - - await burner.recoverERC20(mockERC20Token.address, bn(600000), { from: anotherAccount }) - await burner.recoverERC721(mockNFT.address, nft1, { from: anotherAccount }) - await burner.recoverExcessStETH({ from: anotherAccount }) - }) - it(`REQUEST_BURN_SHARES_ROLE works`, async () => { await web3.eth.sendTransaction({ from: anotherAccount, to: lido.address, value: ETH(2) }) await lido.approve(burner.address, StETH(2), { from: anotherAccount }) @@ -687,7 +649,7 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await lido.balanceOf(burner.address), StETH(7.1)) // should change nothing - const receipt = await burner.recoverExcessStETH({ from: voting }) + const receipt = await burner.recoverExcessStETH({ from: anotherAccount }) assert.emitsNumberOfEvents(receipt, `ExcessStETHRecovered`, 0) // excess stETH amount didn't changed @@ -707,9 +669,9 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await lido.balanceOf(treasury), StETH(0)) const sharesAmount2_3StETH = await lido.sharesOf(burner.address) - const receipt = await burner.recoverExcessStETH({ from: voting }) + const receipt = await burner.recoverExcessStETH({ from: anotherAccount }) assert.emits(receipt, `ExcessStETHRecovered`, { - requestedBy: voting, + requestedBy: anotherAccount, amountOfStETH: StETH(2.3), amountOfShares: sharesAmount2_3StETH, }) @@ -754,9 +716,9 @@ contract('Burner', ([deployer, _, anotherAccount]) => { // run recovery process, excess stETH amount (5) // should be transferred to the treasury const sharesAmount5stETH = await lido.getSharesByPooledEth(StETH(5)) - const receipt = await burner.recoverExcessStETH({ from: voting }) + const receipt = await burner.recoverExcessStETH({ from: anotherAccount }) assert.emits(receipt, `ExcessStETHRecovered`, { - requestedBy: voting, + requestedBy: anotherAccount, amountOfStETH: StETH(5), amountOfShares: sharesAmount5stETH, }) @@ -807,7 +769,7 @@ contract('Burner', ([deployer, _, anotherAccount]) => { }) it(`can't recover zero-address ERC20`, async () => { - await assert.reverts(burner.recoverERC20(ZERO_ADDRESS, bn(10), { from: voting })) + await assert.reverts(burner.recoverERC20(ZERO_ADDRESS, bn(10), { from: anotherAccount })) }) it(`can't recover stETH by recoverERC20`, async () => { @@ -826,17 +788,9 @@ contract('Burner', ([deployer, _, anotherAccount]) => { // revert from anotherAccount // need to use recoverExcessStETH await assert.revertsWithCustomError( - burner.recoverERC20(lido.address, StETH(1), { from: voting }), + burner.recoverERC20(lido.address, StETH(1), { from: anotherAccount }), `StETHRecoveryWrongFunc()` ) - - // revert from deployer - // acl - await assert.revertsOZAccessControl( - burner.recoverERC20(lido.address, StETH(1), { from: deployer }), - deployer, - `RECOVER_ASSETS_ROLE` - ) }) it(`recover some accidentally sent ERC20`, async () => { @@ -849,9 +803,9 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await mockERC20Token.balanceOf(burner.address), bn(600000)) // recover ERC20 - const firstReceipt = await burner.recoverERC20(mockERC20Token.address, bn(100000), { from: voting }) + const firstReceipt = await burner.recoverERC20(mockERC20Token.address, bn(100000), { from: anotherAccount }) assert.emits(firstReceipt, `ERC20Recovered`, { - requestedBy: voting, + requestedBy: anotherAccount, token: mockERC20Token.address, amount: bn(100000), }) @@ -861,24 +815,17 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await mockERC20Token.balanceOf(treasury), bn(100000)) assert.equals(await mockERC20Token.balanceOf(voting), bn(0)) - // acl error - await assert.revertsOZAccessControl( - burner.recoverERC20(mockERC20Token.address, bn(1), { from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - // recover last portion - const lastReceipt = await burner.recoverERC20(mockERC20Token.address, bn(500000), { from: voting }) + const lastReceipt = await burner.recoverERC20(mockERC20Token.address, bn(500000), { from: anotherAccount }) assert.emits(lastReceipt, `ERC20Recovered`, { - requestedBy: voting, + requestedBy: anotherAccount, token: mockERC20Token.address, amount: bn(500000), }) // balance is zero already, have to be reverted await assert.reverts( - burner.recoverERC20(mockERC20Token.address, bn(1), { from: voting }), + burner.recoverERC20(mockERC20Token.address, bn(1), { from: anotherAccount }), `ERC20: transfer amount exceeds balance` ) }) @@ -908,30 +855,20 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await burner.getExcessStETH(), StETH(1)) // can't abuse recoverERC721 API to perform griefing-like attack - await assert.revertsOZAccessControl( - burner.recoverERC721(lido.address, StETH(1), { from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - await assert.revertsOZAccessControl( - burner.recoverERC721(lido.address, StETH(1), { from: deployer }), - deployer, - `RECOVER_ASSETS_ROLE` - ) await assert.revertsWithCustomError( - burner.recoverERC721(lido.address, StETH(1), { from: voting }), + burner.recoverERC721(lido.address, StETH(1), { from: anotherAccount }), `StETHRecoveryWrongFunc()` ) - const receipt = await burner.recoverExcessStETH({ from: voting }) - assert.emits(receipt, `ExcessStETHRecovered`, { requestedBy: voting, amountOfStETH: StETH(1) }) + const receipt = await burner.recoverExcessStETH({ from: anotherAccount }) + assert.emits(receipt, `ExcessStETHRecovered`, { requestedBy: anotherAccount, amountOfStETH: StETH(1) }) // ensure that excess amount is zero assert.equals(await burner.getExcessStETH(), StETH(0)) }) it(`can't recover zero-address ERC721(NFT)`, async () => { - await assert.reverts(burner.recoverERC721(ZERO_ADDRESS, 0, { from: voting })) + await assert.reverts(burner.recoverERC721(ZERO_ADDRESS, 0, { from: anotherAccount })) }) it(`recover some accidentally sent NFTs`, async () => { @@ -944,13 +881,6 @@ contract('Burner', ([deployer, _, anotherAccount]) => { assert.equals(await mockNFT.balanceOf(anotherAccount), bn(1)) assert.equals(await mockNFT.balanceOf(burner.address), bn(1)) - // access control revert - await assert.revertsOZAccessControl( - burner.recoverERC721(mockNFT.address, nft2, { from: anotherAccount }), - anotherAccount, - `RECOVER_ASSETS_ROLE` - ) - // recover nft2 should work const receiptNfc2 = await burner.recoverERC721(mockNFT.address, nft2, { from: voting }) assert.emits(receiptNfc2, `ERC721Recovered`, { requestedBy: voting, token: mockNFT.address, tokenId: nft2 }) diff --git a/test/helpers/factories.js b/test/helpers/factories.js index 374073f43..87ea41d6c 100644 --- a/test/helpers/factories.js +++ b/test/helpers/factories.js @@ -297,14 +297,12 @@ async function guardiansFactory({ deployParams }) { async function burnerFactory({ appManager, treasury, pool, voting }) { const burner = await Burner.new(appManager.address, treasury.address, pool.address, 0, 0) - const [REQUEST_BURN_MY_STETH_ROLE, REQUEST_BURN_SHARES_ROLE, RECOVER_ASSETS_ROLE] = await Promise.all([ + const [REQUEST_BURN_MY_STETH_ROLE, REQUEST_BURN_SHARES_ROLE] = await Promise.all([ burner.REQUEST_BURN_MY_STETH_ROLE(), burner.REQUEST_BURN_SHARES_ROLE(), - burner.RECOVER_ASSETS_ROLE(), ]) await burner.grantRole(REQUEST_BURN_MY_STETH_ROLE, voting.address, { from: appManager.address }) - await burner.grantRole(RECOVER_ASSETS_ROLE, voting.address, { from: appManager.address }) await burner.grantRole(REQUEST_BURN_SHARES_ROLE, voting.address, { from: appManager.address }) return burner From 6db4af668c99e0b34467f252b8ccae5b6c408c16 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 12 Apr 2023 17:19:24 +0400 Subject: [PATCH 3/7] Check arrays length matches in claimWithdrawals --- contracts/0.8.9/WithdrawalQueue.sol | 10 ++++++++++ test/0.8.9/withdrawal-queue.test.js | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/contracts/0.8.9/WithdrawalQueue.sol b/contracts/0.8.9/WithdrawalQueue.sol index 77a2b23f4..fb31f20ce 100644 --- a/contracts/0.8.9/WithdrawalQueue.sol +++ b/contracts/0.8.9/WithdrawalQueue.sol @@ -72,6 +72,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) { @@ -238,6 +239,7 @@ 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 @@ -245,6 +247,9 @@ abstract contract WithdrawalQueue is AccessControlEnumerable, PausableUntil, Wit 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); @@ -257,10 +262,15 @@ abstract contract WithdrawalQueue is AccessControlEnumerable, PausableUntil, Wit /// @param _hints checkpoint hint for each id. /// Can be retrieved 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]); diff --git a/test/0.8.9/withdrawal-queue.test.js b/test/0.8.9/withdrawal-queue.test.js index 49f36fd46..cbf581720 100644 --- a/test/0.8.9/withdrawal-queue.test.js +++ b/test/0.8.9/withdrawal-queue.test.js @@ -597,6 +597,13 @@ contract('WithdrawalQueue', ([owner, stranger, daoAgent, user, pauser, resumer, ) }) + it('reverts when requestIds and hints arrays length mismatch', async () => { + await assert.reverts( + withdrawalQueue.claimWithdrawalsTo([0], [1, 2], user, { from: owner }), + 'ArraysLengthMismatch(1, 2)' + ) + }) + it('reverts with zero _requestId', async () => { await assert.reverts(withdrawalQueue.claimWithdrawalsTo([0], [1], user, { from: owner }), 'InvalidRequestId(0)') }) @@ -722,6 +729,10 @@ contract('WithdrawalQueue', ([owner, stranger, daoAgent, user, pauser, resumer, await withdrawalQueue.requestWithdrawals([amount], owner, { from: user }) }) + it('reverts when requestIds and hints arrays length mismatch', async () => { + await assert.reverts(withdrawalQueue.claimWithdrawals([1, 2], [1], { from: owner }), 'ArraysLengthMismatch(2, 1)') + }) + it('claims correct requests', async () => { await steth.mintShares(owner, shares(300)) // 1 share to user and 299 shares to owner total = 300 ETH await steth.approve(withdrawalQueue.address, StETH(300), { from: owner }) From c956d93fdfe7df80670733b2cae17bb7cda0e434 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 12 Apr 2023 18:05:36 +0400 Subject: [PATCH 4/7] Remove explicit v check from ECDSA lib See details: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591 --- contracts/common/lib/ECDSA.sol | 1 - test/common/lib/signature-utils.test.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/contracts/common/lib/ECDSA.sol b/contracts/common/lib/ECDSA.sol index 3ecac4e37..5e24a39e3 100644 --- a/contracts/common/lib/ECDSA.sol +++ b/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); diff --git a/test/common/lib/signature-utils.test.js b/test/common/lib/signature-utils.test.js index 6ff92d10c..d49a9c51c 100644 --- a/test/common/lib/signature-utils.test.js +++ b/test/common/lib/signature-utils.test.js @@ -63,7 +63,7 @@ function testWithConsumer(SignatureUtilsConsumer, desc) { await assert.reverts( sigUtils.isValidSignature(alice.address, msgHash, INVALID_V, sig.r, sig.s), - `ECDSA: invalid signature 'v' value` + 'ECDSA: invalid signature' ) }) }) From 5a94bcb22fa1e77ace55215b6a3950ce7a8717d5 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Wed, 12 Apr 2023 20:23:45 +0400 Subject: [PATCH 5/7] Forbid zero secondsPerSlot in BaseOracle --- contracts/0.8.9/oracle/BaseOracle.sol | 2 ++ test/0.8.9/oracle/accounting-oracle-deploy.test.js | 4 ++++ test/0.8.9/oracle/base-oracle-deploy.test.js | 5 +++++ .../oracle/validators-exit-bus-oracle-deploy.test.js | 8 ++++++-- 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/contracts/0.8.9/oracle/BaseOracle.sol b/contracts/0.8.9/oracle/BaseOracle.sol index 0cfbead9c..255c1fec5 100644 --- a/contracts/0.8.9/oracle/BaseOracle.sol +++ b/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; } diff --git a/test/0.8.9/oracle/accounting-oracle-deploy.test.js b/test/0.8.9/oracle/accounting-oracle-deploy.test.js index 4e603cf3b..fad53e429 100644 --- a/test/0.8.9/oracle/accounting-oracle-deploy.test.js +++ b/test/0.8.9/oracle/accounting-oracle-deploy.test.js @@ -376,6 +376,10 @@ contract('AccountingOracle', ([admin, member1]) => { await snapshot.rollback() }) + it('reverts when slotsPerSecond is zero', async () => { + await assert.reverts(deployAccountingOracleSetup(admin, { secondsPerSlot: 0 }), 'SecondsPerSlotCannotBeZero()') + }) + it('deployment and init finishes successfully otherwise', async () => { const deployed = await deployAccountingOracleSetup(admin) diff --git a/test/0.8.9/oracle/base-oracle-deploy.test.js b/test/0.8.9/oracle/base-oracle-deploy.test.js index 126bd7bb7..9efb99776 100644 --- a/test/0.8.9/oracle/base-oracle-deploy.test.js +++ b/test/0.8.9/oracle/base-oracle-deploy.test.js @@ -1,5 +1,6 @@ const { contract, artifacts } = require('hardhat') const { bn } = require('@aragon/contract-helpers-test') +const { assert } = require('../../helpers/assert') const BaseOracle = artifacts.require('BaseOracleTimeTravellable') const MockConsensusContract = artifacts.require('MockConsensusContract') @@ -106,6 +107,10 @@ contract('BaseOracle', ([admin]) => { await deployBaseOracle(admin) }) + it('reverts when slotsPerSecond is zero', async () => { + await assert.reverts(deployBaseOracle(admin, { secondsPerSlot: 0 }), 'SecondsPerSlotCannotBeZero()') + }) + // TODO: add more base tests }) }) diff --git a/test/0.8.9/oracle/validators-exit-bus-oracle-deploy.test.js b/test/0.8.9/oracle/validators-exit-bus-oracle-deploy.test.js index b69b28e8f..0ca15d0aa 100644 --- a/test/0.8.9/oracle/validators-exit-bus-oracle-deploy.test.js +++ b/test/0.8.9/oracle/validators-exit-bus-oracle-deploy.test.js @@ -94,11 +94,11 @@ async function deployOracleReportSanityCheckerForExitBus(lidoLocator, admin) { async function deployExitBusOracle( admin, - { dataSubmitter = null, lastProcessingRefSlot = 0, resumeAfterDeploy = false } = {} + { dataSubmitter = null, lastProcessingRefSlot = 0, resumeAfterDeploy = false, secondsPerSlot = SECONDS_PER_SLOT } = {} ) { const locator = (await deployLocatorWithDummyAddressesImplementation(admin)).address - const oracle = await ValidatorsExitBusOracle.new(SECONDS_PER_SLOT, GENESIS_TIME, locator, { from: admin }) + const oracle = await ValidatorsExitBusOracle.new(secondsPerSlot, GENESIS_TIME, locator, { from: admin }) const { consensus } = await deployHashConsensus(admin, { epochsPerFrame: EPOCHS_PER_FRAME, @@ -159,6 +159,10 @@ contract('ValidatorsExitBusOracle', ([admin, member1]) => { oracle = deployed.oracle }) + it('reverts when slotsPerSecond is zero', async () => { + await assert.reverts(deployExitBusOracle(admin, { secondsPerSlot: 0 }), 'SecondsPerSlotCannotBeZero()') + }) + it('mock time-travellable setup is correct', async () => { const time1 = +(await consensus.getTime()) assert.equals(await oracle.getTime(), time1) From a06653ee24754d312c20c1ea38c2c34c95b0606c Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Thu, 13 Apr 2023 02:06:00 +0400 Subject: [PATCH 6/7] Remove payable conversion from _claim --- contracts/0.8.9/WithdrawalQueueBase.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/0.8.9/WithdrawalQueueBase.sol b/contracts/0.8.9/WithdrawalQueueBase.sol index e59f497d1..fbb29d7f4 100644 --- a/contracts/0.8.9/WithdrawalQueueBase.sol +++ b/contracts/0.8.9/WithdrawalQueueBase.sol @@ -481,7 +481,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); } From e9509d77f010fec76899e25ccde785c8de47bd42 Mon Sep 17 00:00:00 2001 From: Bogdan Kovtun Date: Thu, 13 Apr 2023 04:45:47 +0400 Subject: [PATCH 7/7] Doesn't decrease allowance on MAX_UINT256 approval --- contracts/0.4.24/Lido.sol | 4 ++-- contracts/0.4.24/StETH.sol | 27 ++++++++++++++------- test/0.4.24/steth.test.js | 48 ++++++++++++++++++++++++++++++++++---- 3 files changed, 65 insertions(+), 14 deletions(-) diff --git a/contracts/0.4.24/Lido.sol b/contracts/0.4.24/Lido.sol index 086789222..520a9b4ae 100644 --- a/contracts/0.4.24/Lido.sol +++ b/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); diff --git a/contracts/0.4.24/StETH.sol b/contracts/0.4.24/StETH.sol index 681fa0aa6..8a4b40ff6 100644 --- a/contracts/0.4.24/StETH.sol +++ b/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; } @@ -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. */ diff --git a/test/0.4.24/steth.test.js b/test/0.4.24/steth.test.js index 4cd0ca01e..23f5482b3 100644 --- a/test/0.4.24/steth.test.js +++ b/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)) + }) }) }) })