Merge pull request #164 from maticnetwork/dev-audit-fixes

Audit fixes

Author: 0xAshish

contracts/child/ChildERC20.sol

@@ -81,7 +81,7 @@ contract ChildERC20 is BaseERC20, ERC20, ERC20Detailed {
     revert("Disabled feature");
   }
 
-  function transferFrom(address, address, uint256 ) public returns (bool){
+  function transferFrom(address, address, uint256 ) public returns (bool) {
     revert("Disabled feature");
   }
 }

contracts/common/Registry.sol

@@ -78,6 +78,7 @@ contract Registry is Ownable {
   }
 
   function addErc20Predicate(address predicate) public onlyOwner {
+    require(predicate != address(0x0), "Can not add null address as predicate");
     erc20Predicate = predicate;
     addPredicate(predicate, Type.ERC20);
   }
@@ -96,6 +97,7 @@ contract Registry is Ownable {
 
   function removePredicate(address predicate) public onlyOwner
   {
+    require(predicates[predicate]._type != Type.Invalid, "Predicate does not exist");
     delete predicates[predicate];
     emit PredicateRemoved(predicate, msg.sender);
   }

contracts/common/lib/BytesLib.sol

@@ -1,5 +1,6 @@
 pragma solidity ^0.5.2;
 
+import "openzeppelin-solidity/contracts/math/SafeMath.sol";
 
 library BytesLib {
   function concat(
@@ -134,11 +135,13 @@ library BytesLib {
 
   // Pad a bytes array to 32 bytes
   function leftPad(bytes memory _bytes) internal pure returns (bytes memory) {
-    bytes memory newBytes = new bytes(32 - _bytes.length);
+    // may underflow if bytes.length < 32. Hence using SafeMath.sub
+    bytes memory newBytes = new bytes(SafeMath.sub(32, _bytes.length));
     return concat(newBytes, _bytes);
   }
 
   function toBytes32(bytes memory b) internal pure returns (bytes32) {
+    require(b.length >= 32, "Bytes array should atleast be 32 bytes");
     bytes32 out;
     for (uint i = 0; i < 32; i++) {
       out |= bytes32(b[i] & 0xFF) >> (i * 8);
@@ -155,17 +158,14 @@ library BytesLib {
   function fromBytes32(bytes32 x) internal pure returns (bytes memory) {
     bytes memory b = new bytes(32);
     for (uint i = 0; i < 32; i++) {
-      b[i] = byte(uint8(uint(x) / (2**(8*(19 - i)))));
+      b[i] = byte(uint8(uint(x) / (2**(8*(31 - i)))));
     }
     return b;
   }
 
   function fromUint(uint256 _num) internal pure returns (bytes memory _ret) {
-    assembly {
-      _ret := mload(0x10)
-      mstore(_ret, 0x20)
-      mstore(add(_ret, 0x20), _num)
-    }
+    _ret = new bytes(32);
+    assembly { mstore(add(_ret, 32), _num) }
   }
 
   function toUint(bytes memory _bytes, uint _start) internal pure returns (uint256) {

contracts/common/lib/Common.sol

@@ -22,15 +22,6 @@ library Common {
     return (length > 0);
   }
 
-  // convert uint256 to bytes
-  function toBytes(uint256 _num) public pure returns (bytes memory _ret) {
-    assembly {
-      _ret := mload(0x10)
-      mstore(_ret, 0x20)
-      mstore(add(_ret, 0x20), _num)
-    }
-  }
-
   // convert bytes to uint8
   function toUint8(bytes memory _arg) public pure returns (uint8) {
     return uint8(_arg[0]);

contracts/common/lib/Merkle.sol

@@ -10,10 +10,10 @@ library Merkle {
   ) public pure returns (bool) {
     bytes32 proofElement;
     bytes32 computedHash = leaf;
-    uint256 len = (proof.length / 32) * 32;
+    require(proof.length % 32 == 0, "Invalid proof length");
 
     uint256 index = mainIndex;
-    for (uint256 i = 32; i <= len; i += 32) {
+    for (uint256 i = 32; i <= proof.length; i += 32) {
       assembly {
         proofElement := mload(add(proof, i))
       }

contracts/root/withdrawManager/WithdrawManager.sol

@@ -128,14 +128,19 @@ contract WithdrawManager is WithdrawManagerStorage, IWithdrawManager {
       referenceTxData[offset + 1].toBytes() // blockProof
     );
 
+    uint256 _branchMask = branchMask.toRlpItem().toUint();
+    require(
+      _branchMask & 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF00000000 == 0,
+      "Branch mask should be 32 bits"
+    );
     // ageOfInput is denoted as
     // 1 reserve bit (see last 2 lines in comment)
     // 128 bits for exitableAt timestamp
     // 95 bits for child block number
     // 32 bits for receiptPos + logIndex * MAX_LOGS + oIndex
     // In predicates, the exitId will be evaluated by shifting the ageOfInput left by 1 bit
     // (Only in erc20Predicate) Last bit is to differentiate whether the sender or receiver of the in-flight tx is starting an exit
-    return (getExitableAt(createdAt) << 127) | (blockNumber << 32) | branchMask.toRlpItem().toUint();
+    return (getExitableAt(createdAt) << 127) | (blockNumber << 32) | _branchMask;
   }
 
   function startExitWithDepositedTokens(uint256 depositId, address token, uint256 amountOrToken)
@@ -248,14 +253,6 @@ contract WithdrawManager is WithdrawManagerStorage, IWithdrawManager {
     }
   }
 
-  function setExitNFTContract(address _nftContract)
-    external
-    onlyOwner
-  {
-    require(_nftContract != address(0));
-    exitNft = ExitNFT(_nftContract);
-  }
-
   /**
    * @dev Add a state update (UTXO style input) to an exit
    * @param exitId Exit ID

contracts/root/withdrawManager/WithdrawManagerProxy.sol

@@ -4,14 +4,16 @@ import { Registry } from "../../common/Registry.sol";
 import { Proxy } from "../../common/misc/Proxy.sol";
 import { WithdrawManagerStorage } from "./WithdrawManagerStorage.sol";
 import { RootChain } from "../RootChain.sol";
+import { ExitNFT } from "./ExitNFT.sol";
 
 
 contract WithdrawManagerProxy is Proxy, WithdrawManagerStorage {
-  constructor(address _proxyTo, address _registry, address _rootChain)
+  constructor(address _proxyTo, address _registry, address _rootChain, address _exitNft)
     public
     Proxy(_proxyTo)
   {
     registry = Registry(_registry);
     rootChain = RootChain(_rootChain);
+    exitNft = ExitNFT(_exitNft);
   }
 }

contracts/staking/StakeManager.sol

@@ -354,7 +354,7 @@ contract StakeManager is IStakeManager, ERC721Full, RootChainable, Lockable {
     uint256[] memory _validators = new uint256[](validatorThreshold);
     uint256 validator;
     uint256 k = 0;
-    for (uint96 i = 0;i < totalSupply() ;i++) {
+    for (uint256 i = 0; i < totalSupply() ; i++) {
       validator = tokenByIndex(i);
       if (isValidator(validator)) {
         _validators[k++] = validator;
@@ -506,7 +506,7 @@ contract StakeManager is IStakeManager, ERC721Full, RootChainable, Lockable {
   function challangeStateRootUpdate(bytes memory checkpointTx /* txData from submitCheckpoint */) public {
     // TODO: check for 2/3+1 sig and validate non-inclusion in newStateUpdate
     // UPDATE: since we've moved rewards to on chain there is no urgent need for this function
-    // becuase heimdall fee can be trusted on 2/3+1 security 
+    // becuase heimdall fee can be trusted on 2/3+1 security
   }
 
   function _stakeFor(address user, uint256 amount, address signer, bool isContract) internal {

deploy-migrations/2_deploy_root_contracts.js

@@ -146,14 +146,15 @@ module.exports = async function (deployer) {
       RootChain.address
     )
 
+    await deployer.deploy(ExitNFT, Registry.address)
     await deployer.deploy(WithdrawManager)
     await deployer.deploy(
       WithdrawManagerProxy,
       WithdrawManager.address,
       Registry.address,
-      RootChain.address
+      RootChain.address,
+      ExitNFT.address
     )
-    await deployer.deploy(ExitNFT, Registry.address)
 
     console.log('deploying predicates...')
     await deployer.deploy(

deploy-migrations/3_initialize_state.js

@@ -13,7 +13,6 @@ const ERC20Predicate = artifacts.require('ERC20Predicate')
 const ERC721Predicate = artifacts.require('ERC721Predicate')
 const MarketplacePredicate = artifacts.require('MarketplacePredicate')
 const TransferWithSigPredicate = artifacts.require('TransferWithSigPredicate')
-const ExitNFT = artifacts.require('ExitNFT')
 const MaticWeth = artifacts.require('MaticWETH')
 const TestToken = artifacts.require('TestToken')
 
@@ -24,13 +23,11 @@ module.exports = async function (deployer, network) {
       .all([
         TestToken.deployed(),
         Registry.deployed(),
-        RootChain.deployed(),
         DepositManagerProxy.deployed(),
         StateSender.deployed(),
         WithdrawManagerProxy.deployed(),
         StakeManager.deployed(),
         SlashingManager.deployed(),
-        ExitNFT.deployed(),
         MaticWeth.deployed(),
         ERC20Predicate.deployed(),
         ERC721Predicate.deployed(),
@@ -40,23 +37,17 @@ module.exports = async function (deployer, network) {
       .spread(async function (
         testToken,
         registry,
-        rootChain,
         depositManagerProxy,
         stateSender,
         withdrawManagerProxy,
         stakeManager,
         slashingManager,
-        exitNFT,
         maticWeth,
         ERC20Predicate,
         ERC721Predicate,
         MarketplacePredicate,
         TransferWithSigPredicate
       ) {
-        const _withdrawManager = await WithdrawManager.at(
-          withdrawManagerProxy.address
-        )
-        await _withdrawManager.setExitNFTContract(exitNFT.address)
         await registry.updateContractMap(
           ethUtils.keccak256('depositManager'),
           depositManagerProxy.address

moonwalker-migrations/queueJobs.js

@@ -41,7 +41,8 @@ async function deploy() {
   await deployer.deploy(transformArtifact('DepositManagerProxy', ['DepositManager', 'Registry', 'RootChain']))
 
   await deployer.deploy(transformArtifact('WithdrawManager', []))
-  await deployer.deploy(transformArtifact('WithdrawManagerProxy', ['WithdrawManager', 'Registry', 'RootChain']))
+  await deployer.deploy(transformArtifact('ExitNFT', ['Registry']))
+  await deployer.deploy(transformArtifact('WithdrawManagerProxy', ['WithdrawManager', 'Registry', 'RootChain', 'ExitNFT']))
 
   await deployer.deploy(transformArtifact('TestToken', [{ value: 'Test Token' }, { value: 'TST' }]))
 

test/helpers/deployer.js

@@ -152,17 +152,14 @@ class Deployer {
     this.withdrawManagerProxy = await contracts.WithdrawManagerProxy.new(
       this.withdrawManager.address,
       this.registry.address,
-      this.rootChain.address
+      this.rootChain.address,
+      this.exitNFT.address
     )
     await this.registry.updateContractMap(
       ethUtils.keccak256('withdrawManager'),
       this.withdrawManagerProxy.address
     )
-    const w = await contracts.WithdrawManager.at(
-      this.withdrawManagerProxy.address
-    )
-    await w.setExitNFTContract(this.exitNFT.address)
-    return w
+    return contracts.WithdrawManager.at(this.withdrawManagerProxy.address)
   }
 
   async deployErc20Predicate() {