From 8a9db615e91554186f601e6d119bfbbe52ad6e10 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 23 Mar 2020 02:19:43 -0500 Subject: [PATCH 1/5] Remove outdated comment No need for the warning, funcion is no longer empty --- implementation/contracts/DepositLog.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/implementation/contracts/DepositLog.sol b/implementation/contracts/DepositLog.sol index 15367a29a..0413ad682 100644 --- a/implementation/contracts/DepositLog.sol +++ b/implementation/contracts/DepositLog.sol @@ -110,7 +110,6 @@ contract DepositLog { /// We don't require this, so deposits are not bricked if the system borks /// @param _caller The address of the calling contract /// @return True if approved, otherwise false - /* solium-disable-next-line no-empty-blocks */ function approvedToLog(address _caller) public view returns (bool) { return tbtcDepositToken.exists(uint256(_caller)); } From 8ea92a6573b6988c0036d3ad02fbfa4a6e64ad3c Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 23 Mar 2020 02:26:00 -0500 Subject: [PATCH 2/5] Remove tbtcDepositToken from tbtcSystem TBTCSystem and DepositLog hold an independent copy of TBTCDepositToken TbtcSystem extedns DepositLog and theref ore the variable is shodowed. Instead of renaming and keeping both, using only the one from DepositLog is preferable --- implementation/contracts/system/TBTCSystem.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index 44f5da039..60c374cf9 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -47,7 +47,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { uint256 pausedTimestamp; uint256 constant pausedDuration = 10 days; - TBTCDepositToken tbtcDepositToken; address public keepVendor; address public priceFeed; address public relay; @@ -100,7 +99,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { uint256 _keepSize ) external onlyOwner { require(!_initialized, "already initialized"); - tbtcDepositToken = TBTCDepositToken(_tbtcDepositToken); keepVendor = _keepVendor; VendingMachine(_vendingMachine).setExternalAddresses( _tbtcToken, From 6837b39145409e9e20b1bdef3fb839c0fdfd9f7d Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 23 Mar 2020 03:03:22 -0500 Subject: [PATCH 3/5] Remove log return values The return values are not used. On approvedToLog error revert isntead of returning false. Returning false offers the abhlity to not halt the deposit if approvedToLog returns false. This does not fel like a real perk. if approvedToLog returns false the event loger is not the deposit contract, and should revert. --- implementation/contracts/DepositLog.sol | 187 ++++++++++++------------ 1 file changed, 96 insertions(+), 91 deletions(-) diff --git a/implementation/contracts/DepositLog.sol b/implementation/contracts/DepositLog.sol index 0413ad682..c96595fdd 100644 --- a/implementation/contracts/DepositLog.sol +++ b/implementation/contracts/DepositLog.sol @@ -104,12 +104,12 @@ contract DepositLog { // AUTH // - /// @notice Checks if an address is an allowed logger + /// @notice Checks if an address is an allowed logger. /// @dev checks tbtcDepositToken to see if the caller represents /// an existing deposit. - /// We don't require this, so deposits are not bricked if the system borks - /// @param _caller The address of the calling contract - /// @return True if approved, otherwise false + /// We don't require this, so deposits are not bricked if the system borks. + /// @param _caller The address of the calling contract. + /// @return True if approved, otherwise false. function approvedToLog(address _caller) public view returns (bool) { return tbtcDepositToken.exists(uint256(_caller)); } @@ -130,25 +130,27 @@ contract DepositLog { // Logging // - /// @notice Fires a Created event - /// @dev We append the sender, which is the deposit contract that called - /// @param _keepAddress The address of the associated keep - /// @return True if successful, else revert - function logCreated(address _keepAddress) public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a Created event. + /// @dev We append the sender, which is the deposit contract that called. + /// @param _keepAddress The address of the associated keep. + /// @return True if successful, else revert. + function logCreated(address _keepAddress) public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit Created(msg.sender, _keepAddress, block.timestamp); - return true; } - /// @notice Fires a RedemptionRequested event - /// @dev This is the only event without an explicit timestamp - /// @param _requester The ethereum address of the requester - /// @param _digest The calculated sighash digest - /// @param _utxoSize The size of the utxo in sat + /// @notice Fires a RedemptionRequested event. + /// @dev This is the only event without an explicit timestamp. + /// @param _requester The ethereum address of the requester. + /// @param _digest The calculated sighash digest. + /// @param _utxoSize The size of the utxo in sat. /// @param _redeemerOutputScript The redeemer's length-prefixed output script. - /// @param _requestedFee The requester or bump-system specified fee - /// @param _outpoint The 36 byte outpoint - /// @return True if successful, else revert + /// @param _requestedFee The requester or bump-system specified fee. + /// @param _outpoint The 36 byte outpoint. + /// @return True if successful, else revert. function logRedemptionRequested( address _requester, bytes32 _digest, @@ -156,8 +158,11 @@ contract DepositLog { bytes memory _redeemerOutputScript, uint256 _requestedFee, bytes memory _outpoint - ) public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + ) public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit RedemptionRequested( msg.sender, _requester, @@ -167,20 +172,20 @@ contract DepositLog { _requestedFee, _outpoint ); - return true; } - /// @notice Fires a GotRedemptionSignature event - /// @dev We append the sender, which is the deposit contract that called - /// @param _digest signed digest - /// @param _r signature r value - /// @param _s signature s value - /// @return True if successful, else revert + /// @notice Fires a GotRedemptionSignature event. + /// @dev We append the sender, which is the deposit contract that called. + /// @param _digest signed digest. + /// @param _r signature r value. + /// @param _s signature s value. function logGotRedemptionSignature(bytes32 _digest, bytes32 _r, bytes32 _s) public - returns (bool) { - if (!approvedToLog(msg.sender)) return false; + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit GotRedemptionSignature( msg.sender, _digest, @@ -188,104 +193,104 @@ contract DepositLog { _s, block.timestamp ); - return true; } - /// @notice Fires a RegisteredPubkey event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false + /// @notice Fires a RegisteredPubkey event. + /// @dev We append the sender, which is the deposit contract that called. function logRegisteredPubkey( bytes32 _signingGroupPubkeyX, bytes32 _signingGroupPubkeyY - ) public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + ) public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit RegisteredPubkey( msg.sender, _signingGroupPubkeyX, _signingGroupPubkeyY, block.timestamp ); - return true; } - /// @notice Fires a SetupFailed event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logSetupFailed() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a SetupFailed event. + /// @dev We append the sender, which is the deposit contract that called. + function logSetupFailed() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit SetupFailed(msg.sender, block.timestamp); - return true; } - /// @notice Fires a FraudDuringSetup event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logFraudDuringSetup() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a FraudDuringSetup event. + /// @dev We append the sender, which is the deposit contract that called. + function logFraudDuringSetup() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit FraudDuringSetup(msg.sender, block.timestamp); - return true; } - /// @notice Fires a Funded event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logFunded() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a Funded event. + /// @dev We append the sender, which is the deposit contract that called. + function logFunded() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit Funded(msg.sender, block.timestamp); - return true; } - /// @notice Fires a CourtesyCalled event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logCourtesyCalled() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a CourtesyCalled event. + /// @dev We append the sender, which is the deposit contract that called. + function logCourtesyCalled() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit CourtesyCalled(msg.sender, block.timestamp); - return true; } - /// @notice Fires a StartedLiquidation event - /// @dev We append the sender, which is the deposit contract that called - /// @param _wasFraud True if liquidating for fraud - /// @return True if successful, else revert - function logStartedLiquidation(bool _wasFraud) public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @notice Fires a StartedLiquidation event. + /// @dev We append the sender, which is the deposit contract that called. + /// @param _wasFraud True if liquidating for fraud. + function logStartedLiquidation(bool _wasFraud) public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit StartedLiquidation(msg.sender, _wasFraud, block.timestamp); - return true; } /// @notice Fires a Redeemed event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logRedeemed(bytes32 _txid) public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @dev We append the sender, which is the deposit contract that called. + function logRedeemed(bytes32 _txid) public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit Redeemed(msg.sender, _txid, block.timestamp); - return true; } /// @notice Fires a Liquidated event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logLiquidated() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @dev We append the sender, which is the deposit contract that called. + function logLiquidated() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit Liquidated(msg.sender, block.timestamp); - return true; } /// @notice Fires a ExitedCourtesyCall event - /// @dev We append the sender, which is the deposit contract that called - /// returns false if not approved, to prevent accidentally halting Deposit - /// @return True if successful, else false - function logExitedCourtesyCall() public returns (bool) { - if (!approvedToLog(msg.sender)) return false; + /// @dev We append the sender, which is the deposit contract that called. + function logExitedCourtesyCall() public { + require( + approvedToLog(msg.sender), + "Caller is not approved to log events" + ); emit ExitedCourtesyCall(msg.sender, block.timestamp); - return true; } } From 7aad20206707ee16d96fe77cca832df2dbc42251 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 23 Mar 2020 03:03:31 -0500 Subject: [PATCH 4/5] Update tests --- implementation/test/DepositFactoryTest.js | 3 ++- implementation/test/DepositUtilsTest.js | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/implementation/test/DepositFactoryTest.js b/implementation/test/DepositFactoryTest.js index b7d90f6b0..77618dfdb 100644 --- a/implementation/test/DepositFactoryTest.js +++ b/implementation/test/DepositFactoryTest.js @@ -1,5 +1,5 @@ const {deployAndLinkAll} = require("./helpers/testDeployer.js") -const {contract, web3} = require("@openzeppelin/test-environment") +const {contract, web3, accounts} = require("@openzeppelin/test-environment") const {states} = require("./helpers/utils.js") const {BN, constants, expectRevert} = require("@openzeppelin/test-helpers") const {ZERO_ADDRESS} = constants @@ -174,6 +174,7 @@ describe("DepositFactory", async function() { it("is not affected by state changes to master", async () => { const keep = await ECDSAKeepStub.new() + await tbtcDepositToken.forceMint(accounts[0], testDeposit.address) await testDeposit.createNewDeposit( tbtcSystemStub.address, tbtcToken.address, diff --git a/implementation/test/DepositUtilsTest.js b/implementation/test/DepositUtilsTest.js index 155f01102..9725129fd 100644 --- a/implementation/test/DepositUtilsTest.js +++ b/implementation/test/DepositUtilsTest.js @@ -72,6 +72,10 @@ describe("DepositUtils", async function() { beneficiary = accounts[2] feeRebateToken.forceMint(beneficiary, web3.utils.toBN(testDeposit.address)) + tbtcDepositToken.forceMint( + beneficiary, + web3.utils.toBN(testDeposit.address), + ) await testDeposit.createNewDeposit( tbtcSystemStub.address, @@ -280,6 +284,10 @@ describe("DepositUtils", async function() { beneficiary = accounts[2] + tbtcDepositToken.forceMint( + beneficiary, + web3.utils.toBN(testDeposit.address), + ) feeRebateToken.forceMint( beneficiary, web3.utils.toBN(testDeposit.address), From f1810be1eb13950255bc4592ef171a534949139a Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 23 Mar 2020 04:36:52 -0500 Subject: [PATCH 5/5] Make setTbtcDepositToken internal setTbtcDepositToken should only be called internally --- implementation/contracts/DepositLog.sol | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/implementation/contracts/DepositLog.sol b/implementation/contracts/DepositLog.sol index c96595fdd..d355f0915 100644 --- a/implementation/contracts/DepositLog.sol +++ b/implementation/contracts/DepositLog.sol @@ -114,18 +114,6 @@ contract DepositLog { return tbtcDepositToken.exists(uint256(_caller)); } - /// @notice Sets the tbtcDepositToken contract. - /// @dev The contract is used by `approvedToLog` to check if the - /// caller is a Deposit contract. This should only be called once. - /// @param _tbtcDepositTokenAddress The address of the tbtcDepositToken. - function setTbtcDepositToken(address _tbtcDepositTokenAddress) public { - require( - address(tbtcDepositToken) == address(0), - "tbtcDepositToken is already set" - ); - tbtcDepositToken = TBTCDepositToken(_tbtcDepositTokenAddress); - } - // // Logging // @@ -293,4 +281,16 @@ contract DepositLog { ); emit ExitedCourtesyCall(msg.sender, block.timestamp); } + + /// @notice Sets the tbtcDepositToken contract. + /// @dev The contract is used by `approvedToLog` to check if the + /// caller is a Deposit contract. This should only be called once. + /// @param _tbtcDepositTokenAddress The address of the tbtcDepositToken. + function setTbtcDepositToken(address _tbtcDepositTokenAddress) internal { + require( + address(tbtcDepositToken) == address(0), + "tbtcDepositToken is already set" + ); + tbtcDepositToken = TBTCDepositToken(_tbtcDepositTokenAddress); + } }