From a11a29e6987ab0d32fa4be7806215e1459dec9a2 Mon Sep 17 00:00:00 2001 From: noiach Date: Mon, 4 Nov 2019 16:37:58 +1100 Subject: [PATCH] contracts: created CanReclaimTokens --- .../DarknodeRegistry/DarknodeRegistry.sol | 13 ++-------- .../DarknodeRegistryStore.sol | 18 +++---------- contracts/Shifter/ERC20Shifted.sol | 13 ++-------- contracts/Shifter/Shifter.sol | 13 ++-------- contracts/Shifter/ShifterRegistry.sol | 13 ++-------- contracts/libraries/CanReclaimTokens.sol | 25 +++++++++++++++++++ test/DarknodeRegistry.ts | 2 +- 7 files changed, 37 insertions(+), 60 deletions(-) create mode 100644 contracts/libraries/CanReclaimTokens.sol diff --git a/contracts/DarknodeRegistry/DarknodeRegistry.sol b/contracts/DarknodeRegistry/DarknodeRegistry.sol index b60e8276..58d6c1c6 100644 --- a/contracts/DarknodeRegistry/DarknodeRegistry.sol +++ b/contracts/DarknodeRegistry/DarknodeRegistry.sol @@ -6,6 +6,7 @@ import "openzeppelin-solidity/contracts/math/SafeMath.sol"; import "../RenToken/RenToken.sol"; import "./DarknodeRegistryStore.sol"; import "../libraries/Claimable.sol"; +import "../libraries/CanReclaimTokens.sol"; interface IDarknodePaymentStore { } @@ -20,7 +21,7 @@ interface IDarknodeSlasher { /// @notice DarknodeRegistry is responsible for the registration and /// deregistration of Darknodes. -contract DarknodeRegistry is Claimable { +contract DarknodeRegistry is Claimable, CanReclaimTokens { using SafeMath for uint256; string public VERSION; // Passed in as a constructor parameter. @@ -169,16 +170,6 @@ contract DarknodeRegistry is Claimable { numDarknodesPreviousEpoch = 0; } - /// @notice Allow the owner of the contract to recover funds accidentally - /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. - function recoverTokens(address _token) external onlyOwner { - if (_token == address(0x0)) { - msg.sender.transfer(address(this).balance); - } else { - ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); - } - } - /// @notice Register a darknode and transfer the bond to this contract. /// Before registering, the bond transfer must be approved in the REN /// contract. The caller must provide a public encryption key for the diff --git a/contracts/DarknodeRegistry/DarknodeRegistryStore.sol b/contracts/DarknodeRegistry/DarknodeRegistryStore.sol index cb63a188..c95834df 100644 --- a/contracts/DarknodeRegistry/DarknodeRegistryStore.sol +++ b/contracts/DarknodeRegistry/DarknodeRegistryStore.sol @@ -5,11 +5,12 @@ import "openzeppelin-solidity/contracts/math/SafeMath.sol"; import "../libraries/Claimable.sol"; import "../libraries/LinkedList.sol"; import "../RenToken/RenToken.sol"; +import "../libraries/CanReclaimTokens.sol"; /// @notice This contract stores data and funds for the DarknodeRegistry /// contract. The data / fund logic and storage have been separated to improve /// upgradability. -contract DarknodeRegistryStore is Claimable { +contract DarknodeRegistryStore is Claimable, CanReclaimTokens { using SafeMath for uint256; string public VERSION; // Passed in as a constructor parameter. @@ -61,20 +62,7 @@ contract DarknodeRegistryStore is Claimable { ) public { VERSION = _VERSION; ren = _ren; - } - - /// @notice Allow the owner of the contract to recover funds accidentally - /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. - /// @dev The owner is the Darknode Registry so it would need to be updated - /// first before being able to recover funds. - function recoverTokens(address _token) external onlyOwner { - require(_token != address(ren), "DarknodeRegistryStore: not allowed to recover REN"); - - if (_token == address(0x0)) { - msg.sender.transfer(address(this).balance); - } else { - ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); - } + blacklistRecoverableToken(address(ren)); } /// @notice Instantiates a darknode and appends it to the darknodes diff --git a/contracts/Shifter/ERC20Shifted.sol b/contracts/Shifter/ERC20Shifted.sol index 54840309..fa43bb93 100644 --- a/contracts/Shifter/ERC20Shifted.sol +++ b/contracts/Shifter/ERC20Shifted.sol @@ -4,25 +4,16 @@ import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol"; import "openzeppelin-solidity/contracts/token/ERC20/ERC20Detailed.sol"; import "../libraries/Claimable.sol"; +import "../libraries/CanReclaimTokens.sol"; /// @notice ERC20Shifted represents a digital asset that has been bridged on to /// the Ethereum ledger. It exposes mint and burn functions that can only be /// called by it's associated Shifter. -contract ERC20Shifted is ERC20, ERC20Detailed, Claimable { +contract ERC20Shifted is ERC20, ERC20Detailed, Claimable, CanReclaimTokens { /* solium-disable-next-line no-empty-blocks */ constructor(string memory _name, string memory _symbol, uint8 _decimals) public ERC20Detailed(_name, _symbol, _decimals) {} - /// @notice Allow the owner of the contract to recover funds accidentally - /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. - function recoverTokens(address _token) external onlyOwner { - if (_token == address(0x0)) { - msg.sender.transfer(address(this).balance); - } else { - ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); - } - } - function burn(address _from, uint256 _amount) public onlyOwner { _burn(_from, _amount); } diff --git a/contracts/Shifter/Shifter.sol b/contracts/Shifter/Shifter.sol index ef5ccacf..48c09023 100644 --- a/contracts/Shifter/Shifter.sol +++ b/contracts/Shifter/Shifter.sol @@ -7,11 +7,12 @@ import "openzeppelin-solidity/contracts/cryptography/ECDSA.sol"; import "../libraries/Claimable.sol"; import "../libraries/String.sol"; import "./ERC20Shifted.sol"; +import "../libraries/CanReclaimTokens.sol"; /// @notice Shifter handles verifying mint and burn requests. A mintAuthority /// approves new assets to be minted by providing a digital signature. An owner /// of an asset can request for it to be burnt. -contract Shifter is Claimable { +contract Shifter is Claimable, CanReclaimTokens { using SafeMath for uint256; uint8 public version = 2; @@ -74,16 +75,6 @@ contract Shifter is Claimable { updateFeeRecipient(_feeRecipient); } - /// @notice Allow the owner of the contract to recover funds accidentally - /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. - function recoverTokens(address _token) external onlyOwner { - if (_token == address(0x0)) { - msg.sender.transfer(address(this).balance); - } else { - ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); - } - } - // Public functions //////////////////////////////////////////////////////// /// @notice Claims ownership of the token passed in to the constructor. diff --git a/contracts/Shifter/ShifterRegistry.sol b/contracts/Shifter/ShifterRegistry.sol index d2f459e1..3c38ea45 100644 --- a/contracts/Shifter/ShifterRegistry.sol +++ b/contracts/Shifter/ShifterRegistry.sol @@ -4,10 +4,11 @@ import "../libraries/Claimable.sol"; import "./ERC20Shifted.sol"; import "../libraries/LinkedList.sol"; import "./IShifter.sol"; +import "../libraries/CanReclaimTokens.sol"; /// @notice ShifterRegistry is a mapping from assets to their associated /// ERC20Shifted and Shifter contracts. -contract ShifterRegistry is Claimable { +contract ShifterRegistry is Claimable, CanReclaimTokens { /// @dev The symbol is included twice because strings have to be hashed /// first in order to be used as a log index/topic. @@ -30,16 +31,6 @@ contract ShifterRegistry is Claimable { /// @notice A map of token symbols to canonical shifter addresses mapping (string=>address) private tokenBySymbol; - /// @notice Allow the owner of the contract to recover funds accidentally - /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. - function recoverTokens(address _token) external onlyOwner { - if (_token == address(0x0)) { - msg.sender.transfer(address(this).balance); - } else { - ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); - } - } - /// @notice Allow the owner to set the shifter address for a given /// ERC20Shifted token contract. /// diff --git a/contracts/libraries/CanReclaimTokens.sol b/contracts/libraries/CanReclaimTokens.sol new file mode 100644 index 00000000..d326794f --- /dev/null +++ b/contracts/libraries/CanReclaimTokens.sol @@ -0,0 +1,25 @@ +pragma solidity ^0.5.12; + +import "openzeppelin-solidity/contracts/ownership/Ownable.sol"; +import "openzeppelin-solidity/contracts/token/ERC20/ERC20.sol"; + +contract CanReclaimTokens is Ownable { + + mapping(address => bool) private recoverableTokensBlacklist; + + function blacklistRecoverableToken(address _token) public { + recoverableTokensBlacklist[_token] = true; + } + + /// @notice Allow the owner of the contract to recover funds accidentally + /// sent to the contract. To withdraw ETH, the token should be set to `0x0`. + function recoverTokens(address _token) external onlyOwner { + require(!recoverableTokensBlacklist[_token], "CanReclaimTokens: token is not recoverable"); + + if (_token == address(0x0)) { + msg.sender.transfer(address(this).balance); + } else { + ERC20(_token).transfer(msg.sender, ERC20(_token).balanceOf(address(this))); + } + } +} diff --git a/test/DarknodeRegistry.ts b/test/DarknodeRegistry.ts index 5e324bb7..d83f4e88 100644 --- a/test/DarknodeRegistry.ts +++ b/test/DarknodeRegistry.ts @@ -687,7 +687,7 @@ contract("DarknodeRegistry", (accounts: string[]) => { // Can't recover REN await dnrs.recoverTokens(ren.address, { from: accounts[0] }) - .should.be.rejectedWith(/DarknodeRegistryStore: not allowed to recover REN/); + .should.be.rejectedWith(/CanReclaimTokens: token is not recoverable/); // Only the owner can recover tokens await dnrs.recoverTokens(token.address, { from: accounts[1] })