ethernautas / renpool Goto Github PK

The following entries in .gitignore reference difference network settings

Are they being used? I guess the only value we might need in the PRIVATE_KEY, which changes across networks. But for now we only use it on Kovan.

One of the issue reported by Slither is about reentrancy on the deposit method.

Reentrancy in RenPool.deposit(uint256) (contracts/RenPool.sol#133-149):
        External calls:
        - require(bool,string)(renToken.transferFrom(sender,address(this),_amount),RenPool: Deposit failed) (contracts/RenPool.sol#143)
        State variables written after the call(s):
        - _lockPool() (contracts/RenPool.sol#147)
                - isLocked = true (contracts/RenPool.sol#115)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-1

Verify that this is an actual issue, and remove the warning from Slither.

Review current project branches to check whether they have become stale. If so, we should delete them.

Related PR #51. However, quite outdated since we have migrated to Hardhat in the meantime.

Given that the contract has been migrated from Vyper to Solidity, and that GitHub recognizes Solidity source files out of the box https://github.com/github/linguist/blob/master/lib/linguist/languages.yml#L5745, we might not need .gitattributes file.

If that's the case, we can remove .gitattributes.

We expect these tests to continue growing. Placing them in the same file can result in major merge conflicts. To avoid this issue, we should separate the tests into multiple files.

Here's is a possible way how to solve it
https://www.alxolr.com/articles/how-to-separate-mocha-tests-in-multiple-files.

With this issue, we start the migration to HardHat, from Brownie.

The HardHat project should be able to compile the RenPool contract properly. This means, it should be configured with a suitable compiler and OpenZeppelin dependencies.

The HardHat project should use yarn to manage its dependencies.

After the initial migration phase #56, migrate deploy script to Hardhat.

After the initial migration phase #56, migrate register darknode script to Hardhat.

With a receive or fallback function we are able to pay for gas when withdrawing from the protocol.

Refer to https://docs.soliditylang.org/en/v0.6.2/contracts.html#receive-ether-function for implementation details.

Here are the logs when running github actions:

tests/conftest.py:32: in <module>
Brownie v1.14.6 - Python development framework for Ethereum
    network.connect(net)

Compiling contracts...
  Solc version: 0.8.7
  Optimizer: Enabled  Runs: 200
  EVM Version: Istanbul
Generating build data...
 - AccessControl
 - IAccessControl
 - MerkleProof
 - EIP712
 - ConditionalEscrow
 - Escrow
 - RefundEscrow
 - ERC165
 - ERC165Checker
 - ERC165Storage
 - ERC1820Implementer
 - IERC165
 - IERC1820Implementer
 - IERC1820Registry
 - Math
 - SafeCast
 - SafeMath
 - SignedSafeMath
 - EnumerableMap
 - EnumerableSet

New compatible vyper version available: 0.2.12
Compiling contracts...
  Vyper version: 0.2.12
Generating build data...
 - RenPool_old

Compiling contracts...
  Solc version: 0.8.7
  Optimizer: Enabled  Runs: 200
  EVM Version: Istanbul
Generating build data...
 - OpenZeppelin/[email protected]/ERC20
 - OpenZeppelin/[email protected]/IERC20
 - OpenZeppelin/[email protected]/Context
 - RenPool
 - RenToken
 - IDarknodeRegistry

Generating interface ABIs...

Launching 'ganache-cli --port 8545 --gasLimit 12000000 --accounts 10 --hardfork istanbul --mnemonic brownie --fork https://mainnet.infura.io/v3/*** --chainId 1'...
Error: Process completed with exit code 4.

Slither is a static analyzer for Solidity smart contracts. It detects potential errors, vulnerabilities and promote best practices.

Integrate Slither into our CI.

Usually a new repo use main as its default branch name, instead of using master. If we adhere to this convention, we should rename the default branch.

Under the Ren Pool project, there are several cards. Convert any actionable, i.e., something needs to be implemented, card into an issue. We should delete the remaining cards to have a clean backlog.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://consensys.github.io/smart-contract-best-practices/known_attacks/

The darknodes dashboard is found here https://testnet.renproject.io/darknodes

Currently RenPool contract implements its own ownership mechanism. This mechanism is already implemented by OpenZeppelin. Use the Ownable contract to simplify RenPool's logic.

See https://docs.openzeppelin.com/contracts/4.x/access-control for reference.

There is no tracking mechanism for an Owner or Node Operator to see their deployed RenPool contracts.

Introduce a new contract, RenPoolFactory, to keep track of all deployed pools in a central contract.

As mentioned in https://blog.openzeppelin.com/openzeppelin-contracts-4-0/

[...] Solidity 0.8 marks the end of the SafeMath era, as overflow checks are now built into the compiler.

You will find that SafeMath is still in the repository, athough it is now just a wrapper over the built-in overflow checks.

I see no point in use SafeMath. Am I missing something @fede-rodes?

After successful migration to Hardhat has been completed, some Brownie related files still remain in the repo. Remove this files as they are no longer needed.

We might need to call refund first.

Source: https://github.com/renproject/darknode-sol/blob/master/contracts/DarknodeRegistry/DarknodeRegistry.sol#L271

https://github.com/renproject/darknode-sol/blob/master/contracts/DarknodeRegistry/DarknodeRegistry.sol#L479

Source: https://github.com/renproject/darknode-sol/blob/master/contracts/DarknodeRegistry/DarknodeRegistry.sol#L232

Kovan address : https://kovan.etherscan.io/address/0x9954C9F839b31E82bc9CA98F234313112D269712

We need to implement the base58 to hex conversion in order to process the node ID given by the darknode CLI.

For instance, when running the darknode cli you get the following output:

Congratulations! Your Darknode is deployed.
Join the network by registering your Darknode at https://mainnet.renproject.io/darknode/8MGTMfNPeekDnSrJDqmf5oSVZaK3Vx?action=register&public_key=0x000000077373682d727361000000030100010000010100d0feba4ae65ea9ad771d153419bcc21189d954b6bf75fd5488055cd2641231014f190c0e059a452d301c535e931df33590ec0e18c59341a2766cc885d1dc6e66f5cc65b94522ec944ae4200bd56a30223328b258d50b507dd94b4c4742768f3fec2b815c9c4b0fe26727e82865f6a064fa3ff2443d135d9788095a1c17487fd5c389a491c16b73385d516a303debc3bcccae337a7ec0d89d51ce05262a0c4c1f2178466c85379b8cd4e5cbe1c90a05fb0c1ed3eee2134774b450e7b0b70c792abad55beef919e21a03cb9de4e963a820c2f84421a4559d0b67cfd17c1686ff6f2d1bb07ac2c82cede1cf5f16a57e125a29fef65891715b061606bca1a0eb026b&name=darknode-kovan

If I understand correctly the 8MGTMfNPeekDnSrJDqmf5oSVZaK3Vx is the nodeID which is required when registering the node (together with the publicKey). Now, in order for the register function to work the nodeID needs to be an address. Therefore we should convert from base58ToHex.

See how the conversion is done in the command center:
https://github.com/renproject/command-center/blob/9aa947a6fd1ec3459c06809e4fb82915032805a6/src/lib/darknode/darknodeID.ts#L7

Here's the register function of the DarknodeRegistry:
https://github.com/Ethernautas/darknode-sol/blob/master/contracts/DarknodeRegistry/DarknodeRegistry.sol#L232

And here where we should use the encoding:
https://github.com/Ethernautas/renpool/blob/master/tests/ren_pool/test_darknode_registration.py#L6

Current README contains instructions on how to run the project using Brownie. After the migration to Hardhat, update README to describe Hardhat instructions.

Currently, this is not possible due to a limitation in Hardhat. See NomicFoundation/hardhat#1111.

Once this is resolved, we can set up multiple forking networks.