Merge pull request #78 from endaoment/audit-l05

Audit Item L05
endaoment · Aug 8, 2020 · 1d3e029 · 1d3e029
2 parents f5c32d2 + 1bb8963
commit 1d3e029
Show file tree

Hide file tree

Showing 20 changed files with 339 additions and 278 deletions.
diff --git a/README.md b/README.md
@@ -50,6 +50,9 @@ The `EndaomentAdmin` contract creates the Roles necessary to execute the oversig
 
 Several "getter" functions are also available to allow for querying of current role status or holder. 
 
+### `EndaomentAdminStorage.sol`
+To prevent an attacker from deploying their own EndaomentAdmin contract and representing themselves as the EndaomentAdmin, both `FundFactory` and `OrgFactory` use `EndaomentAdminStorage` to allow `Fund` and `Org` contracts to keep a record of the current and correct `EndaomentAdmin` contract for overseeing actions. 
+
 ### `Administratable.sol`
 Provides an interface containing key modifiers for administering the `FundFactory` and `OrgFactory` that reference the `EndaomentAdmin` contract. 
 

diff --git a/contracts/Administratable.sol b/contracts/Administratable.sol
@@ -4,8 +4,11 @@ pragma solidity ^0.6.10;
 
 import "./EndaomentAdmin.sol";
 
+//ADMINISTRATABLE
 /**
- * @dev Provides two modifiers allowing contracts administered
+ * @title Administratable
+ * @author rheeger
+ * @notice Provides two modifiers allowing contracts administered
  * by the EndaomentAdmin contract to properly restrict method calls
  * based on the a given role. Also provides a utility function for
  * validating string input arguments.
@@ -33,18 +36,20 @@ contract Administratable {
    * @notice onlyAdminOrRole checks that the caller is either the Admin or the provided role.
    * @param adminContractAddress supplied EndaomentAdmin address
    * @param role The role to require unless the caller is the owner. Permitted
-   * roles are admin (0), accountant (2), and reviewer (3).
+   * roles are ADMIN (6), ACCOUNTANT (2), REVIEWER (3), FUND_FACTORY (4) and ORG_FACTORY(5).
    */
   modifier onlyAdminOrRole(address adminContractAddress, IEndaomentAdmin.Role role) {
     _onlyAdminOrRole(adminContractAddress, role);
     _;
   }
-
-  function _onlyAdminOrRole(address adminContractAddress, IEndaomentAdmin.Role role) 
-    private
-    view
-  {
 
+  /**
+   * @notice _onlyAdminOrRole checks that the caller is either the Admin or the provided role.
+   * @param adminContractAddress supplied EndaomentAdmin address
+   * @param role The role to require unless the caller is the owner. Permitted
+   * roles are ADMIN (6), ACCOUNTANT (2), REVIEWER (3), FUND_FACTORY (4) and ORG_FACTORY(5).
+   */
+  function _onlyAdminOrRole(address adminContractAddress, IEndaomentAdmin.Role role) private view {
     require(
       adminContractAddress != address(0),
       "Administratable: Admin must not be the zero address"
@@ -83,22 +88,32 @@ contract Administratable {
       }
     }
   }
-
-  // TODO(rheeger): write docs in audit-l05
-  modifier onlyAddressOrAdminOrRole(address allowedAddress, address adminContractAddress, IEndaomentAdmin.Role role) {
+
+  /**
+   * @notice Checks that the caller is either a provided adress, admin or role.
+   * @param allowedAddress An exempt address provided that shall be allowed to proceed.
+   * @param adminContractAddress The EndaomentAdmin contract address.
+   * @param role The desired IEndaomentAdmin.Role to check against. Permitted
+   * roles are ADMIN (6), ACCOUNTANT (2), REVIEWER (3), FUND_FACTORY (4) and ORG_FACTORY(5).
+   */
+  modifier onlyAddressOrAdminOrRole(
+    address allowedAddress,
+    address adminContractAddress,
+    IEndaomentAdmin.Role role
+  ) {
     require(
       allowedAddress != address(0),
       "Administratable: Allowed address must not be the zero address"
     );
-   
+
     bool isAllowed = (msg.sender == allowedAddress);
 
     if (!isAllowed) {
-        _onlyAdminOrRole(adminContractAddress, role);      
+      _onlyAdminOrRole(adminContractAddress, role);
     }
     _;
   }
-  
+
   /**
    * @notice Returns true if two strings are equal, false otherwise
    * @param s1 First string to compare

diff --git a/contracts/EndaomentAdmin.sol b/contracts/EndaomentAdmin.sol
@@ -104,6 +104,13 @@ contract TwoStepOwnable {
   }
 }
 
+/**
+ * @title EndaomentAdmin
+ * @author rheeger
+ * @notice Provides admin controls for the Endaoment contract ecosystem using
+ * a roles-based system. Available roles are PAUSER (1), ACCOUNTANT (2),
+ * REVIEWER (3), FUND_FACTORY (4), ORG_FACTORY (5), and ADMIN (6).
+ */
 contract EndaomentAdmin is IEndaomentAdmin, TwoStepOwnable {
   // Maintain a role status mapping with assigned accounts and paused states.
   mapping(uint256 => RoleStatus) private _roles;
@@ -181,7 +188,8 @@ contract EndaomentAdmin is IEndaomentAdmin, TwoStepOwnable {
   /**
    * @notice External view function to check the account currently holding the
    * given role.
-   * @return The address of the current admin, or the null
+   * @param role The desired role to fetch the current address of.
+   * @return The address of the requested role, or the null
    * address if none is set.
    */
   function getRoleAddress(Role role) external override view returns (address) {
@@ -231,8 +239,7 @@ contract EndaomentAdmin is IEndaomentAdmin, TwoStepOwnable {
    * @notice Modifier that throws if called by any account other than the owner
    * or the supplied role, or if the caller is not the owner and the role in
    * question is paused.
-   * @param role The role to require unless the caller is the owner. Permitted
-   * roles are bot commander (0) and pauser (1).
+   * @param role The role to require unless the caller is the owner.
    */
   modifier onlyAdminOr(Role role) {
     if (!isOwner()) {

diff --git a/contracts/EndaomentAdminStorage.sol b/contracts/EndaomentAdminStorage.sol
@@ -4,12 +4,20 @@ pragma solidity ^0.6.10;
 
 import "./Administratable.sol";
 
+// ENDAOMENT ADMIN STORAGE CONTRACT
+/**
+ * @title EndaomentAdminStorage
+ * @author rheeger
+ * @notice Stores the contract address of the EndaomentAdmin,
+ * for use in references by the Org and Fund factories and
+ * subsequently deployed Org and Fund contracts.
+ */
 contract EndaomentAdminStorage is Administratable {
   address public endaomentAdmin;
   event EndaomentAdminChanged(address indexed oldAddress, address indexed newAddress);
 
   /**
-   * @notice Update address of the endaomentAdmin contract
+   * @notice Updates address of the endaomentAdmin contract and emits `EndaomentAdminChanged` event.
    * @param newAdmin New address of the endaomentAdmin contract
    */
   function updateEndaomentAdmin(address newAdmin) public onlyAdmin(endaomentAdmin) {

diff --git a/contracts/Fund.sol b/contracts/Fund.sol
@@ -5,7 +5,7 @@ pragma experimental ABIEncoderV2;
 
 import "./Administratable.sol";
 import "./OrgFactory.sol";
-import "./IFactory.sol";
+import "./interfaces/IFactory.sol";
 import "@openzeppelin/contracts/math/SafeMath.sol";
 
 // FUND CONTRACT
@@ -24,7 +24,7 @@ contract Fund is Administratable {
   using SafeERC20 for IERC20;
 
   // ========== STRUCTS & EVENTS ==========
-  
+
   struct Grant {
     string description;
     uint256 value;
@@ -59,7 +59,7 @@ contract Fund is Administratable {
 
   // ========== Fund Management & Info ==========
   /**
-   * @notice Change Fund Primary Advisor
+   * @notice Changes Fund Primary Advisor and emits a `ManagerChanged` event
    * @param  newManager The address of the new PrimaryAdvisor.
    */
   function changeManager(address newManager)
@@ -75,6 +75,7 @@ contract Fund is Administratable {
    * @notice Checks recipient of a Grant is an address created by the OrgFactory
    * @param  recipient The address of the Grant recipient.
    * @param  orgFactoryContractAddress Address of the OrgFactory contract.
+   * @return Boolean of status of given recipient status.
    */
   function checkRecipient(address recipient, address orgFactoryContractAddress)
     public
@@ -90,16 +91,10 @@ contract Fund is Administratable {
 
   /**
    * @notice Returns summary of details about the fund [tokenBalance, number of grants, managerAddress].
-   * @param  tokenAddress The token address of the stablecoin being used by the web-server.
+   * @param  tokenAddress The token address of the ERC20 being used by the web-server.
+   * @return Returns the token balance of the given tokenAddress and the address of the Fund's manager.
    */
-  function getSummary(address tokenAddress)
-    external
-    view
-    returns (
-      uint256,
-      address
-    )
-  {
+  function getSummary(address tokenAddress) external view returns (uint256, address) {
     require(tokenAddress != address(0), "Fund: Token address cannot be the zero address");
     IERC20 tokenContract = IERC20(tokenAddress);
     uint256 balance = tokenContract.balanceOf(address(this));
@@ -108,7 +103,7 @@ contract Fund is Administratable {
   }
 
   /**
-   * @notice Create new Grant Recommendation
+   * @notice Creates new Grant Recommendation and emits a `GrantCreated` event.
    * @param  grantId UUID representing this grant
    * @param  description The address of the Owner.
    * @param  value The value of the grant in base units.
@@ -119,18 +114,23 @@ contract Fund is Administratable {
     string calldata description,
     uint256 value,
     address recipient
-  ) public onlyAddressOrAdminOrRole(manager, fundFactoryContract.endaomentAdmin(), IEndaomentAdmin.Role.REVIEWER) {
+  )
+    public
+    onlyAddressOrAdminOrRole(
+      manager,
+      fundFactoryContract.endaomentAdmin(),
+      IEndaomentAdmin.Role.REVIEWER
+    )
+  {
     require(!isEqual(grantId, ""), "Fund: Must provide a grantId");
     require(!isEqual(description, ""), "Fund: Must provide a description");
     EndaomentAdmin endaomentAdmin = EndaomentAdmin(fundFactoryContract.endaomentAdmin());
     require(
-      checkRecipient(recipient, endaomentAdmin.getRoleAddress(IEndaomentAdmin.Role.ORG_FACTORY)) == true,
+      checkRecipient(recipient, endaomentAdmin.getRoleAddress(IEndaomentAdmin.Role.ORG_FACTORY)) ==
+        true,
       "Fund: Recipient contract was not created by the OrgFactory and is not allowed."
     );
-    require(
-      pendingGrants[grantId].recipient == address(0),
-      "Fund: Grant was already created."
-    );
+    require(pendingGrants[grantId].recipient == address(0), "Fund: Grant was already created.");
 
     Grant memory newGrant = Grant({
       description: description,
@@ -140,10 +140,10 @@ contract Fund is Administratable {
     });
     emit GrantCreated(grantId, newGrant);
     pendingGrants[grantId] = newGrant;
-  } 
+  }
 
   /**
-   * @notice Update Grant Recommendation
+   * @notice Updates Grant Recommendation and emits a `GrantUpdated` event.
    * @param  grantId UUID representing this grant
    * @param  description The address of the Owner.
    * @param  value The value of the grant in base units.
@@ -154,21 +154,24 @@ contract Fund is Administratable {
     string calldata description,
     uint256 value,
     address recipient
-  ) public onlyAddressOrAdminOrRole(manager, fundFactoryContract.endaomentAdmin(), IEndaomentAdmin.Role.REVIEWER) {
+  )
+    public
+    onlyAddressOrAdminOrRole(
+      manager,
+      fundFactoryContract.endaomentAdmin(),
+      IEndaomentAdmin.Role.REVIEWER
+    )
+  {
     require(!isEqual(grantId, ""), "Fund: Must provide a grantId");
     require(!isEqual(description, ""), "Fund: Must provide a description");
     EndaomentAdmin endaomentAdmin = EndaomentAdmin(fundFactoryContract.endaomentAdmin());
     require(
-      checkRecipient(recipient, endaomentAdmin.getRoleAddress(IEndaomentAdmin.Role.ORG_FACTORY)) == true,
+      checkRecipient(recipient, endaomentAdmin.getRoleAddress(IEndaomentAdmin.Role.ORG_FACTORY)) ==
+        true,
       "Fund: Recipient contract was not created by the OrgFactory and is not allowed."
     );
-    require(
-      pendingGrants[grantId].recipient != address(0),
-      "Fund: Grant does not exist."
-    );
-    require(pendingGrants[grantId].complete == false,
-    "Fund: Grant is already finalized."
-    );
+    require(pendingGrants[grantId].recipient != address(0), "Fund: Grant does not exist.");
+    require(pendingGrants[grantId].complete == false, "Fund: Grant is already finalized.");
     Grant memory replacementGrant = Grant({
       description: description,
       value: value,
@@ -180,34 +183,34 @@ contract Fund is Administratable {
   }
 
   /**
-   * @notice Reject Grant Recommendation
+   * @notice Rejects Grant Recommendation and emits a `GrantRejected` event.
    * @param  grantId UUID representing this grant
    */
-  function rejectGrant(
-    string calldata grantId
-  ) public onlyAddressOrAdminOrRole(manager, fundFactoryContract.endaomentAdmin(), IEndaomentAdmin.Role.REVIEWER) {
+  function rejectGrant(string calldata grantId)
+    public
+    onlyAddressOrAdminOrRole(
+      manager,
+      fundFactoryContract.endaomentAdmin(),
+      IEndaomentAdmin.Role.REVIEWER
+    )
+  {
     require(!isEqual(grantId, ""), "Fund: Must provide a grantId");
-    require(
-      pendingGrants[grantId].recipient != address(0),
-      "Fund: Grant does not exist."
-    );
-    require(pendingGrants[grantId].complete == false,
-    "Fund: Grant is already finalized."
-    );
-
+    require(pendingGrants[grantId].recipient != address(0), "Fund: Grant does not exist.");
+    require(pendingGrants[grantId].complete == false, "Fund: Grant is already finalized.");
+
     delete pendingGrants[grantId];
     emit GrantRejected(grantId);
   }
 
   /**
-   * @notice Approve Grant Recommendation
+   * @notice Approves Grant Recommendation and emits a `GrantFinalized` event.
    * @param  grantId UUID of the grant being finalized
-   * @param  tokenAddress The stablecoin's token address.
+   * @param  tokenAddress The ERC20 token address of the token prescribed by the web-server.
    */
-  function finalizeGrant(
-    string calldata grantId,
-    address tokenAddress
-  ) public onlyAdminOrRole(fundFactoryContract.endaomentAdmin(), IEndaomentAdmin.Role.ACCOUNTANT) {
+  function finalizeGrant(string calldata grantId, address tokenAddress)
+    public
+    onlyAdminOrRole(fundFactoryContract.endaomentAdmin(), IEndaomentAdmin.Role.ACCOUNTANT)
+  {
     require(!isEqual(grantId, ""), "Fund: Must provide a grantId");
     require(tokenAddress != address(0), "Fund: Token address cannot be the zero address");
     Grant storage grant = pendingGrants[grantId];
@@ -216,14 +219,14 @@ contract Fund is Administratable {
     require(grant.complete == false, "Fund: Grant is already finalized.");
     // Effects
     IERC20 tokenContract = IERC20(tokenAddress);
-
     // Process fees:
     uint256 fee = grant.value.div(100);
     uint256 finalGrant = grant.value.sub(fee);
     grant.complete = true;
     emit GrantFinalized(grantId, grant);
     // Interactions
-    address endaomentAdminAdminAddress = EndaomentAdmin(fundFactoryContract.endaomentAdmin()).getRoleAddress(IEndaomentAdmin.Role.ADMIN);
+    address endaomentAdminAdminAddress = EndaomentAdmin(fundFactoryContract.endaomentAdmin())
+      .getRoleAddress(IEndaomentAdmin.Role.ADMIN);
     tokenContract.safeTransfer(endaomentAdminAdminAddress, fee);
     tokenContract.safeTransfer(grant.recipient, finalGrant);
   }

diff --git a/contracts/FundFactory.sol b/contracts/FundFactory.sol
@@ -9,10 +9,8 @@ import "./Fund.sol";
 /**
  * @title FundFactory
  * @author rheeger
- * @notice FundFactory is a contract that allows the EndaomentAdmin to
- * instantiate new Fund contracts. It also provides for fetching of
- * individual Org contract addresses as well as a list of all
- * allowedOrgs.
+ * @notice FundFactory is a contract that allows the Endaoment ADMIN or ACCOUNTANT to
+ * instantiate new Fund contracts.
  */
 contract FundFactory is EndaomentAdminStorage {
   // ========== EVENTS ==========
@@ -31,7 +29,7 @@ contract FundFactory is EndaomentAdminStorage {
 
   // ========== Fund Creation & Management ==========
   /**
-   * @notice Creates new Fund and emits FundCreated event.
+   * @notice Creates new Fund and emits a `FundCreated` event.
    * @param managerAddress The address of the Fund's Primary Advisor
    */
   function createFund(address managerAddress)