From 5518aaa41d4de489be7bfd190e15550e86464717 Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Tue, 4 Feb 2020 15:05:13 +0530 Subject: [PATCH 1/3] Misc audit fixes --- contracts/child/ChildERC20.sol | 2 +- contracts/common/Registry.sol | 2 ++ contracts/common/lib/BytesLib.sol | 14 +++++++------- contracts/common/lib/Common.sol | 7 ++----- contracts/common/lib/Merkle.sol | 5 +++-- .../root/withdrawManager/WithdrawManager.sol | 15 ++++++--------- .../root/withdrawManager/WithdrawManagerProxy.sol | 4 +++- contracts/staking/StakeManager.sol | 4 ++-- deploy-migrations/2_deploy_root_contracts.js | 5 +++-- deploy-migrations/3_initialize_state.js | 9 --------- test/helpers/deployer.js | 9 +++------ 11 files changed, 32 insertions(+), 44 deletions(-) diff --git a/contracts/child/ChildERC20.sol b/contracts/child/ChildERC20.sol index 436892c3d..c3cd3aaa4 100644 --- a/contracts/child/ChildERC20.sol +++ b/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"); } } diff --git a/contracts/common/Registry.sol b/contracts/common/Registry.sol index 7a7ec5c0f..43f1392dd 100644 --- a/contracts/common/Registry.sol +++ b/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); } diff --git a/contracts/common/lib/BytesLib.sol b/contracts/common/lib/BytesLib.sol index 6632df44e..864ecd555 100644 --- a/contracts/common/lib/BytesLib.sol +++ b/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) { diff --git a/contracts/common/lib/Common.sol b/contracts/common/lib/Common.sol index fb8024151..d6471d8e2 100644 --- a/contracts/common/lib/Common.sol +++ b/contracts/common/lib/Common.sol @@ -24,11 +24,8 @@ library Common { // 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) - } + _ret = new bytes(32); + assembly { mstore(add(_ret, 32), _num) } } // convert bytes to uint8 diff --git a/contracts/common/lib/Merkle.sol b/contracts/common/lib/Merkle.sol index 55cb9247d..7f3c3fd93 100644 --- a/contracts/common/lib/Merkle.sol +++ b/contracts/common/lib/Merkle.sol @@ -10,10 +10,11 @@ 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 len = (proof.length / 32) * 32; 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)) } diff --git a/contracts/root/withdrawManager/WithdrawManager.sol b/contracts/root/withdrawManager/WithdrawManager.sol index f4af3d748..4f8ea4a46 100644 --- a/contracts/root/withdrawManager/WithdrawManager.sol +++ b/contracts/root/withdrawManager/WithdrawManager.sol @@ -128,6 +128,11 @@ 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 @@ -135,7 +140,7 @@ contract WithdrawManager is WithdrawManagerStorage, IWithdrawManager { // 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 diff --git a/contracts/root/withdrawManager/WithdrawManagerProxy.sol b/contracts/root/withdrawManager/WithdrawManagerProxy.sol index ec8d24338..110b2643b 100644 --- a/contracts/root/withdrawManager/WithdrawManagerProxy.sol +++ b/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); } } diff --git a/contracts/staking/StakeManager.sol b/contracts/staking/StakeManager.sol index 8d79b8cf7..de8c5103b 100644 --- a/contracts/staking/StakeManager.sol +++ b/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 { diff --git a/deploy-migrations/2_deploy_root_contracts.js b/deploy-migrations/2_deploy_root_contracts.js index 9e5c3712d..91f3a7742 100644 --- a/deploy-migrations/2_deploy_root_contracts.js +++ b/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( diff --git a/deploy-migrations/3_initialize_state.js b/deploy-migrations/3_initialize_state.js index be3421d8c..e6e97ba8e 100644 --- a/deploy-migrations/3_initialize_state.js +++ b/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 diff --git a/test/helpers/deployer.js b/test/helpers/deployer.js index 96dcf126a..d84e4a9c4 100644 --- a/test/helpers/deployer.js +++ b/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() { From b86b7b23f24b07f99d4305e27197488fda43743d Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Wed, 5 Feb 2020 17:47:16 +0530 Subject: [PATCH 2/3] Remove redundant function --- contracts/common/lib/Common.sol | 6 ------ contracts/common/lib/Merkle.sol | 1 - 2 files changed, 7 deletions(-) diff --git a/contracts/common/lib/Common.sol b/contracts/common/lib/Common.sol index d6471d8e2..00f2d717c 100644 --- a/contracts/common/lib/Common.sol +++ b/contracts/common/lib/Common.sol @@ -22,12 +22,6 @@ library Common { return (length > 0); } - // convert uint256 to bytes - function toBytes(uint256 _num) public pure returns (bytes memory _ret) { - _ret = new bytes(32); - assembly { mstore(add(_ret, 32), _num) } - } - // convert bytes to uint8 function toUint8(bytes memory _arg) public pure returns (uint8) { return uint8(_arg[0]); diff --git a/contracts/common/lib/Merkle.sol b/contracts/common/lib/Merkle.sol index 7f3c3fd93..74e4f30d5 100644 --- a/contracts/common/lib/Merkle.sol +++ b/contracts/common/lib/Merkle.sol @@ -11,7 +11,6 @@ library Merkle { bytes32 proofElement; bytes32 computedHash = leaf; require(proof.length % 32 == 0, "Invalid proof length"); - // uint256 len = (proof.length / 32) * 32; uint256 index = mainIndex; for (uint256 i = 32; i <= proof.length; i += 32) { From d5aa322f12dca817d8139b26e5183793c9c179fa Mon Sep 17 00:00:00 2001 From: atvanguard <93arpit@gmail.com> Date: Wed, 5 Feb 2020 18:01:49 +0530 Subject: [PATCH 3/3] Update moonwalker migration for withdrawManager --- moonwalker-migrations/queueJobs.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/moonwalker-migrations/queueJobs.js b/moonwalker-migrations/queueJobs.js index e86c6abfc..33cf78e59 100644 --- a/moonwalker-migrations/queueJobs.js +++ b/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' }]))