Optimize Bridge gas usage (#212)

* Apply various optimizations from #201 * Reformat code * Use local AccessContract in ERC721MinterBurnerPauser * Add natspec for AccessControl.getRoleMemberIndex() * Add more annotations about custom AccessControl * Fix withDepositer test * Bump solidity version to 0.6.12 * Add SafeCast tests * Remove excessive dependencies * Fix uint8 and uint32 SafeCast conditions * Update proposal expiration related checks
ChainSafe · Nov 23, 2020 · 8979e9e · 8979e9e
1 parent 6e98831
commit 8979e9e
Show file tree

Hide file tree

Showing 29 changed files with 479 additions and 175 deletions.
diff --git a/contracts/Bridge.sol b/contracts/Bridge.sol
@@ -1,9 +1,10 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
-import "@openzeppelin/contracts/access/AccessControl.sol";
+import "./utils/AccessControl.sol";
 import "./utils/Pausable.sol";
 import "./utils/SafeMath.sol";
+import "./utils/SafeCast.sol";
 import "./interfaces/IDepositExecute.sol";
 import "./interfaces/IERCHandler.sol";
 import "./interfaces/IGenericHandler.sol";
@@ -13,56 +14,51 @@ import "./interfaces/IGenericHandler.sol";
     @author ChainSafe Systems.
  */
 contract Bridge is Pausable, AccessControl, SafeMath {
+    using SafeCast for *;
+
+    // Limit relayers number because proposal can fit only so much votes
+    uint256 constant public MAX_RELAYERS = 200;
 
     uint8   public _chainID;
-    uint256 public _relayerThreshold;
-    uint256 public _totalProposals;
-    uint256 public _fee;
-    uint256 public _expiry;
+    uint8   public _relayerThreshold;
+    uint128 public _fee;
+    uint40  public _expiry;
 
     enum ProposalStatus {Inactive, Active, Passed, Executed, Cancelled}
 
     struct Proposal {
-        bytes32 _resourceID;
-        bytes32 _dataHash;
-        address[] _yesVotes;
-        address[] _noVotes;
         ProposalStatus _status;
-        uint256 _proposedBlock;
+        uint200 _yesVotes;      // bitmap, 200 maximum votes
+        uint8   _yesVotesTotal;
+        uint40  _proposedBlock; // 1099511627775 maximum block
     }
 
     // destinationChainID => number of deposits
     mapping(uint8 => uint64) public _depositCounts;
     // resourceID => handler address
     mapping(bytes32 => address) public _resourceIDToHandlerAddress;
-    // depositNonce => destinationChainID => bytes
-    mapping(uint64 => mapping(uint8 => bytes)) public _depositRecords;
     // destinationChainID + depositNonce => dataHash => Proposal
-    mapping(uint72 => mapping(bytes32 => Proposal)) public _proposals;
-    // destinationChainID + depositNonce => dataHash => relayerAddress => bool
-    mapping(uint72 => mapping(bytes32 => mapping(address => bool))) public _hasVotedOnProposal;
+    mapping(uint72 => mapping(bytes32 => Proposal)) private _proposals;
 
-    event RelayerThresholdChanged(uint256 indexed newThreshold);
-    event RelayerAdded(address indexed relayer);
-    event RelayerRemoved(address indexed relayer);
+    event RelayerThresholdChanged(uint256 newThreshold);
+    event RelayerAdded(address relayer);
+    event RelayerRemoved(address relayer);
     event Deposit(
-        uint8   indexed destinationChainID,
-        bytes32 indexed resourceID,
-        uint64  indexed depositNonce
+        uint8   destinationChainID,
+        bytes32 resourceID,
+        uint64  depositNonce
     );
     event ProposalEvent(
-        uint8           indexed originChainID,
-        uint64          indexed depositNonce,
-        ProposalStatus  indexed status,
-        bytes32 resourceID,
+        uint8          originChainID,
+        uint64         depositNonce,
+        ProposalStatus status,
         bytes32 dataHash
     );
-
     event ProposalVote(
-        uint8   indexed originChainID,
-        uint64  indexed depositNonce,
-        ProposalStatus indexed status,
-        bytes32 resourceID
+        uint8   originChainID,
+        uint64  depositNonce,
+        ProposalStatus status,
+        bytes32 dataHash
     );
 
     bytes32 public constant RELAYER_ROLE = keccak256("RELAYER_ROLE");
@@ -95,6 +91,14 @@ contract Bridge is Pausable, AccessControl, SafeMath {
         require(hasRole(RELAYER_ROLE, msg.sender), "sender doesn't have relayer role");
     }
 
+    function _relayerBit(address relayer) private view returns(uint) {
+        return uint(1) << sub(AccessControl.getRoleMemberIndex(RELAYER_ROLE, relayer), 1);
+    }
+
+    function _hasVoted(Proposal memory proposal, address relayer) private view returns(bool) {
+        return (_relayerBit(relayer) & uint(proposal._yesVotes)) > 0;
+    }
+
     /**
         @notice Initializes Bridge, creates and grants {msg.sender} the admin role,
         creates and grants {initialRelayers} the relayer role.
@@ -104,16 +108,26 @@ contract Bridge is Pausable, AccessControl, SafeMath {
      */
     constructor (uint8 chainID, address[] memory initialRelayers, uint256 initialRelayerThreshold, uint256 fee, uint256 expiry) public {
         _chainID = chainID;
-        _relayerThreshold = initialRelayerThreshold;
-        _fee = fee;
-        _expiry = expiry;
+        _relayerThreshold = initialRelayerThreshold.toUint8();
+        _fee = fee.toUint128();
+        _expiry = expiry.toUint40();
 
         _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
 
         for (uint256 i; i < initialRelayers.length; i++) {
             grantRole(RELAYER_ROLE, initialRelayers[i]);
         }
+    }
 
+    /**
+        @notice Returns true if {relayer} has voted on {destNonce} {dataHash} proposal.
+        @notice Naming left unchanged for backward compatibility.
+        @param destNonce destinationChainID + depositNonce of the proposal.
+        @param dataHash Hash of data to be provided when deposit proposal is executed.
+        @param relayer Address to check.
+     */
+    function _hasVotedOnProposal(uint72 destNonce, bytes32 dataHash, address relayer) public view returns(bool) {
+        return _hasVoted(_proposals[destNonce][dataHash], relayer);
     }
 
     /**
@@ -158,7 +172,7 @@ contract Bridge is Pausable, AccessControl, SafeMath {
         @notice Emits {RelayerThresholdChanged} event.
      */
     function adminChangeRelayerThreshold(uint256 newThreshold) external onlyAdmin {
-        _relayerThreshold = newThreshold;
+        _relayerThreshold = newThreshold.toUint8();
         emit RelayerThresholdChanged(newThreshold);
     }
 
@@ -171,6 +185,7 @@ contract Bridge is Pausable, AccessControl, SafeMath {
      */
     function adminAddRelayer(address relayerAddress) external {
         require(!hasRole(RELAYER_ROLE, relayerAddress), "addr already has relayer role!");
+        require(_totalRelayers() < MAX_RELAYERS, "relayers limit reached");
         grantRole(RELAYER_ROLE, relayerAddress);
         emit RelayerAdded(relayerAddress);
     }
@@ -254,7 +269,7 @@ contract Bridge is Pausable, AccessControl, SafeMath {
         @notice Returns total relayers number.
         @notice Added for backwards compatibility.
      */
-    function _totalRelayers() external view returns (uint) {
+    function _totalRelayers() public view returns (uint) {
         return AccessControl.getRoleMemberCount(RELAYER_ROLE);
     }
 
@@ -265,7 +280,7 @@ contract Bridge is Pausable, AccessControl, SafeMath {
      */
     function adminChangeFee(uint256 newFee) external onlyAdmin {
         require(_fee != newFee, "Current fee is equal to new fee");
-        _fee = newFee;
+        _fee = newFee.toUint128();
     }
 
     /**
@@ -300,7 +315,6 @@ contract Bridge is Pausable, AccessControl, SafeMath {
         require(handler != address(0), "resourceID not mapped to handler");
 
         uint64 depositNonce = ++_depositCounts[destinationChainID];
-        _depositRecords[depositNonce][destinationChainID] = data;
 
         IDepositExecute depositHandler = IDepositExecute(handler);
         depositHandler.deposit(resourceID, destinationChainID, depositNonce, msg.sender, data);
@@ -322,51 +336,43 @@ contract Bridge is Pausable, AccessControl, SafeMath {
     function voteProposal(uint8 chainID, uint64 depositNonce, bytes32 resourceID, bytes32 dataHash) external onlyRelayers whenNotPaused {
 
         uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(chainID);
-        Proposal storage proposal = _proposals[nonceAndID][dataHash];
+        Proposal memory proposal = _proposals[nonceAndID][dataHash];
 
         require(_resourceIDToHandlerAddress[resourceID] != address(0), "no handler for resourceID");
         require(uint(proposal._status) <= 1, "proposal already passed/executed/cancelled");
-        require(!_hasVotedOnProposal[nonceAndID][dataHash][msg.sender], "relayer already voted");
-
-        if (uint(proposal._status) == 0) {
-            ++_totalProposals;
-            _proposals[nonceAndID][dataHash] = Proposal({
-                _resourceID : resourceID,
-                _dataHash : dataHash,
-                _yesVotes : new address[](1),
-                _noVotes : new address[](0),
-                _status : ProposalStatus.Active,
-                _proposedBlock : block.number
-                });
-
-            proposal._yesVotes[0] = msg.sender;
-            emit ProposalEvent(chainID, depositNonce, ProposalStatus.Active, resourceID, dataHash);
-        } else {
-            if (sub(block.number, proposal._proposedBlock) > _expiry) {
-                // if the number of blocks that has passed since this proposal was
-                // submitted exceeds the expiry threshold set, cancel the proposal
-                proposal._status = ProposalStatus.Cancelled;
-                emit ProposalEvent(chainID, depositNonce, ProposalStatus.Cancelled, resourceID, dataHash);
-            } else {
-                require(dataHash == proposal._dataHash, "datahash mismatch");
-                proposal._yesVotes.push(msg.sender);
-
-
-            }
+        require(!_hasVoted(proposal, msg.sender), "relayer already voted");
 
+        if (proposal._status == ProposalStatus.Inactive) {
+            proposal = Proposal({
+                _status : ProposalStatus.Active,
+                _yesVotes : 0,
+                _yesVotesTotal : 0,
+                _proposedBlock : uint40(block.number) // Overflow is desired.
+            });
+
+            emit ProposalEvent(chainID, depositNonce, ProposalStatus.Active, dataHash);
+        } else if (uint40(sub(block.number, proposal._proposedBlock)) > _expiry) {
+            // if the number of blocks that has passed since this proposal was
+            // submitted exceeds the expiry threshold set, cancel the proposal
+            proposal._status = ProposalStatus.Cancelled;
+
+            emit ProposalEvent(chainID, depositNonce, ProposalStatus.Cancelled, dataHash);
         }
+
         if (proposal._status != ProposalStatus.Cancelled) {
-            _hasVotedOnProposal[nonceAndID][dataHash][msg.sender] = true;
-            emit ProposalVote(chainID, depositNonce, proposal._status, resourceID);
+            proposal._yesVotes = (proposal._yesVotes | _relayerBit(msg.sender)).toUint200();
+            proposal._yesVotesTotal++; // TODO: check if bit counting is cheaper.
+
+            emit ProposalVote(chainID, depositNonce, proposal._status, dataHash);
 
             // Finalize if _relayerThreshold has been reached
-            if (proposal._yesVotes.length >= _relayerThreshold) {
+            if (proposal._yesVotesTotal >= _relayerThreshold) {
                 proposal._status = ProposalStatus.Passed;
 
-                emit ProposalEvent(chainID, depositNonce, ProposalStatus.Passed, resourceID, dataHash);
+                emit ProposalEvent(chainID, depositNonce, ProposalStatus.Passed, dataHash);
             }
         }
-
+        _proposals[nonceAndID][dataHash] = proposal;
     }
 
     /**
@@ -380,15 +386,17 @@ contract Bridge is Pausable, AccessControl, SafeMath {
      */
     function cancelProposal(uint8 chainID, uint64 depositNonce, bytes32 dataHash) public onlyAdminOrRelayer {
         uint72 nonceAndID = (uint72(depositNonce) << 8) | uint72(chainID);
-        Proposal storage proposal = _proposals[nonceAndID][dataHash];
+        Proposal memory proposal = _proposals[nonceAndID][dataHash];
         ProposalStatus currentStatus = proposal._status;
 
         require(currentStatus == ProposalStatus.Active || currentStatus == ProposalStatus.Passed,
             "Proposal cannot be cancelled");
-        require(sub(block.number, proposal._proposedBlock) > _expiry, "Proposal not at expiry threshold");
+        require(uint40(sub(block.number, proposal._proposedBlock)) > _expiry, "Proposal not at expiry threshold");
 
         proposal._status = ProposalStatus.Cancelled;
-        emit ProposalEvent(chainID, depositNonce, ProposalStatus.Cancelled, proposal._resourceID, dataHash);
+        _proposals[nonceAndID][dataHash] = proposal;
+
+        emit ProposalEvent(chainID, depositNonce, ProposalStatus.Cancelled, dataHash);
     }
 
     /**
@@ -409,14 +417,13 @@ contract Bridge is Pausable, AccessControl, SafeMath {
         Proposal storage proposal = _proposals[nonceAndID][dataHash];
 
         require(proposal._status == ProposalStatus.Passed, "Proposal must have Passed status");
-        require(dataHash == proposal._dataHash, "Data doesn't match datahash");
 
         proposal._status = ProposalStatus.Executed;
 
         IDepositExecute depositHandler = IDepositExecute(handler);
-        depositHandler.executeProposal(proposal._resourceID, data);
+        depositHandler.executeProposal(resourceID, data);
 
-        emit ProposalEvent(chainID, depositNonce, ProposalStatus.Executed, resourceID, dataHash);
+        emit ProposalEvent(chainID, depositNonce, ProposalStatus.Executed, dataHash);
     }
 
     /**
@@ -430,5 +437,4 @@ contract Bridge is Pausable, AccessControl, SafeMath {
             addrs[i].transfer(amounts[i]);
         }
     }
-
 }
diff --git a/contracts/CentrifugeAsset.sol b/contracts/CentrifugeAsset.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
 /**

diff --git a/contracts/ERC20Safe.sol b/contracts/ERC20Safe.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 import "@openzeppelin/contracts/math/SafeMath.sol";
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

diff --git a/contracts/ERC721MinterBurnerPauser.sol b/contracts/ERC721MinterBurnerPauser.sol
@@ -1,8 +1,8 @@
-pragma solidity ^0.6.0;
+pragma solidity 0.6.12;
 
 // This is adapted from https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v3.0.0/contracts/presets/ERC721PresetMinterPauserAutoId.sol
 
-import "@openzeppelin/contracts/access/AccessControl.sol";
+import "./utils/AccessControl.sol";
 import "@openzeppelin/contracts/GSN/Context.sol";
 import "@openzeppelin/contracts/utils/Counters.sol";
 import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

diff --git a/contracts/ERC721Safe.sol b/contracts/ERC721Safe.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 import "@openzeppelin/contracts/math/SafeMath.sol";
 import "@openzeppelin/contracts/token/ERC721/IERC721.sol";

diff --git a/contracts/Migrations.sol b/contracts/Migrations.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 contract Migrations {
   address public owner;

diff --git a/contracts/TestContracts.sol b/contracts/TestContracts.sol
@@ -1,4 +1,6 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
+
+import "./utils/SafeCast.sol";
 
 contract NoArgument {
     event NoArgumentCalled();
@@ -39,3 +41,11 @@ contract WithDepositer {
         emit WithDepositerCalled(argumentOne, argumentTwo);
     }
 }
+
+contract SafeCaster {
+    using SafeCast for *;
+
+    function toUint200(uint input) external pure returns(uint200) {
+        return input.toUint200();
+    }
+}
diff --git a/contracts/handlers/ERC20Handler.sol b/contracts/handlers/ERC20Handler.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
 import "../interfaces/IDepositExecute.sol";

diff --git a/contracts/handlers/ERC721Handler.sol b/contracts/handlers/ERC721Handler.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
 import "../interfaces/IDepositExecute.sol";

diff --git a/contracts/handlers/GenericHandler.sol b/contracts/handlers/GenericHandler.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 pragma experimental ABIEncoderV2;
 
 import "../interfaces/IGenericHandler.sol";

diff --git a/contracts/handlers/HandlerHelpers.sol b/contracts/handlers/HandlerHelpers.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 import "../interfaces/IERCHandler.sol";
 

diff --git a/contracts/interfaces/IBridge.sol b/contracts/interfaces/IBridge.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 /**
     @title Interface for Bridge contract.

diff --git a/contracts/interfaces/IDepositExecute.sol b/contracts/interfaces/IDepositExecute.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 /**
     @title Interface for handler contracts that support deposits and deposit executions.

diff --git a/contracts/interfaces/IERCHandler.sol b/contracts/interfaces/IERCHandler.sol
@@ -1,4 +1,4 @@
-pragma solidity 0.6.4;
+pragma solidity 0.6.12;
 
 /**
     @title Interface to be used with handlers that support ERC20s and ERC721s.