Fix Least Authority Issues (#66)

* Remove unused code (#2)

* Remove redundant lines in OverlayV1Token constructor. (4)

* Replace _setupRole with _grantRole. (2)

* Simplify toInt192Bounded (3)

* Rounding down of snapAccumulator might influence calculations (#41)

contracts/OverlayV1Token.sol

@@ -8,11 +8,7 @@ import "./interfaces/IOverlayV1Token.sol";
 
 contract OverlayV1Token is IOverlayV1Token, AccessControlEnumerable, ERC20("Overlay", "OVL") {
     constructor() {
-        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
-        _setupRole(MINTER_ROLE, msg.sender);
-        _setRoleAdmin(MINTER_ROLE, DEFAULT_ADMIN_ROLE);
-        _setRoleAdmin(BURNER_ROLE, DEFAULT_ADMIN_ROLE);
-        _setRoleAdmin(GOVERNOR_ROLE, DEFAULT_ADMIN_ROLE);
+        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
     }
 
     modifier onlyMinter() {

contracts/feeds/uniswapv3/OverlayV1UniswapV3Feed.sol

@@ -280,7 +280,6 @@ contract OverlayV1UniswapV3Feed is IOverlayV1UniswapV3Feed, OverlayV1Feed {
         harmonicMeanLiquidities_ = new uint128[](nowIdxsLength);
 
         for (uint256 i = 0; i < nowIdxsLength; i++) {
-            uint32 secondsAgo = secondsAgos[i];
             uint256 nowIdx = nowIdxs[i];
             uint32 window = windows[i];
 

contracts/libraries/Cast.sol

@@ -12,9 +12,9 @@ library Cast {
     /// @dev casts an int256 to an int192 bounded by int192 range of values
     /// @dev to avoid reverts and overflows
     function toInt192Bounded(int256 value) internal pure returns (int192) {
-        int192 value192 = (type(int192).min <= value && value <= type(int192).max)
-            ? int192(value)
-            : (value < type(int192).min ? type(int192).min : type(int192).max);
+        int192 value192 = value < type(int192).min
+            ? type(int192).min
+            : (value > type(int192).max ? type(int192).max : int192(value));
         return value192;
     }
 }

contracts/libraries/Roller.sol

@@ -53,12 +53,7 @@ library Roller {
         // otherwise, calculate fraction of value remaining given linear decay.
         // fraction of value to take off due to decay (linear drift toward zero)
         // is fraction of windowLast that has elapsed since timestampLast
-        uint256 windowFraction = dt.divDown(snapWindow);
-        uint256 absSnapAccumulator = snapAccumulator.abs();
-        int256 dSnapAccumulator = snapAccumulator >= 0
-            ? int256(windowFraction.mulDown(absSnapAccumulator))
-            : -int256(windowFraction.mulDown(absSnapAccumulator));
-        snapAccumulator -= dSnapAccumulator;
+        snapAccumulator = (snapAccumulator * int256(snapWindow - dt)) / int256(snapWindow);
 
         // add in the new value for accumulator now
         int256 accumulatorNow = snapAccumulator + value;

scripts/deploy.py

@@ -58,4 +58,3 @@ def main():
 
     # renounce admin rights so only gov has roles
     ovl.renounceRole(_role(ADMIN), dev, {"from": dev})
-    ovl.renounceRole(_role(MINTER), dev, {"from": dev})

tests/deployers/market/conftest.py

@@ -1,5 +1,5 @@
 import pytest
-from brownie import OverlayV1Deployer, OverlayV1FeedMock, OverlayV1Token
+from brownie import web3, OverlayV1Deployer, OverlayV1FeedMock, OverlayV1Token
 
 
 @pytest.fixture(scope="module")
@@ -27,13 +27,33 @@ def rando(accounts):
     yield accounts[4]
 
 
+@pytest.fixture(scope="module")
+def minter_role():
+    yield web3.solidityKeccak(['string'], ["MINTER"])
+
+
+@pytest.fixture(scope="module")
+def burner_role():
+    yield web3.solidityKeccak(['string'], ["BURNER"])
+
+
+@pytest.fixture(scope="module")
+def governor_role():
+    yield web3.solidityKeccak(['string'], ["GOVERNOR"])
+
+
 @pytest.fixture(scope="module", params=[8000000])
-def create_token(gov, alice, bob, request):
+def create_token(gov, alice, bob, minter_role, request):
     sup = request.param
 
     def create_token(supply=sup):
         tok = gov.deploy(OverlayV1Token)
+
+        # mint the token then renounce minter role
+        tok.grantRole(minter_role, gov, {"from": gov})
         tok.mint(gov, supply * 10 ** tok.decimals(), {"from": gov})
+        tok.renounceRole(minter_role, gov, {"from": gov})
+
         tok.transfer(alice, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         tok.transfer(bob, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         return tok

tests/factories/market/conftest.py

@@ -52,12 +52,17 @@ def governor_role():
 
 
 @pytest.fixture(scope="module", params=[8000000])
-def create_token(gov, alice, bob, request):
+def create_token(gov, alice, bob, minter_role, request):
     sup = request.param
 
     def create_token(supply=sup):
         tok = gov.deploy(OverlayV1Token)
+
+        # mint the token then renounce minter role
+        tok.grantRole(minter_role, gov, {"from": gov})
         tok.mint(gov, supply * 10 ** tok.decimals(), {"from": gov})
+        tok.renounceRole(minter_role, gov, {"from": gov})
+
         tok.transfer(alice, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         tok.transfer(bob, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         return tok

tests/markets/conftest.py

@@ -52,12 +52,17 @@ def governor_role():
 
 
 @pytest.fixture(scope="module", params=[8000000])
-def create_token(gov, alice, bob, request):
+def create_token(gov, alice, bob, minter_role, request):
     sup = request.param
 
     def create_token(supply=sup):
         tok = gov.deploy(OverlayV1Token)
+
+        # mint the token then renounce minter role
+        tok.grantRole(minter_role, gov, {"from": gov})
         tok.mint(gov, supply * 10 ** tok.decimals(), {"from": gov})
+        tok.renounceRole(minter_role, gov, {"from": gov})
+
         tok.transfer(alice, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         tok.transfer(bob, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         return tok

tests/token/conftest.py

@@ -38,12 +38,17 @@ def governor_role():
 
 
 @pytest.fixture(scope="module", params=[8000000])
-def create_token(gov, alice, bob, request):
+def create_token(gov, alice, bob, minter_role, request):
     sup = request.param
 
     def create_token(supply=sup):
         tok = gov.deploy(OverlayV1Token)
+
+        # mint the token then renounce minter role
+        tok.grantRole(minter_role, gov, {"from": gov})
         tok.mint(gov, supply * 10 ** tok.decimals(), {"from": gov})
+        tok.renounceRole(minter_role, gov, {"from": gov})
+
         tok.transfer(alice, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         tok.transfer(bob, (supply/2) * 10 ** tok.decimals(), {"from": gov})
         return tok
@@ -84,11 +89,25 @@ def burner(create_burner):
     yield create_burner()
 
 
+@pytest.fixture(scope="module")
+def create_governor(token, gov, accounts, governor_role):
+    def create_governor(tok=token, governance=gov):
+        tok.grantRole(governor_role, accounts[6], {"from": gov})
+        return accounts[6]
+
+    yield create_governor
+
+
+@pytest.fixture(scope="module")
+def governor(create_governor):
+    yield create_governor()
+
+
 @pytest.fixture(scope="module")
 def create_admin(token, gov, accounts):
     def create_admin(tok=token, governance=gov):
-        tok.grantRole(tok.DEFAULT_ADMIN_ROLE(), accounts[6], {"from": gov})
-        return accounts[6]
+        tok.grantRole(tok.DEFAULT_ADMIN_ROLE(), accounts[7], {"from": gov})
+        return accounts[7]
 
     yield create_admin
 
@@ -101,9 +120,9 @@ def admin(create_admin):
 @pytest.fixture(scope="module")
 def create_market(token, admin, accounts, minter_role, burner_role):
     def create_market(tok=token, adm=admin):
-        tok.grantRole(minter_role, accounts[7], {"from": adm})
-        tok.grantRole(burner_role, accounts[7], {"from": adm})
-        return accounts[7]
+        tok.grantRole(minter_role, accounts[8], {"from": adm})
+        tok.grantRole(burner_role, accounts[8], {"from": adm})
+        return accounts[8]
 
     yield create_market
 

tests/token/test_conftest.py

@@ -7,12 +7,12 @@ def test_balances(token, gov, alice, bob, minter, burner, admin):
     assert token.balanceOf(admin) == 0
 
 
-def test_roles(token, gov, minter, burner, admin, market,
-               rando, minter_role, burner_role):
+def test_roles(token, gov, minter, burner, governor, admin, market,
+               rando, minter_role, burner_role, governor_role):
     assert token.hasRole(token.DEFAULT_ADMIN_ROLE(), gov) is True
     assert token.hasRole(minter_role, minter) is True
     assert token.hasRole(burner_role, burner) is True
-
+    assert token.hasRole(governor_role, governor) is True
     assert token.hasRole(token.DEFAULT_ADMIN_ROLE(), admin) is True
 
     assert token.hasRole(minter_role, market) is True
@@ -21,6 +21,13 @@ def test_roles(token, gov, minter, burner, admin, market,
     assert token.hasRole(token.DEFAULT_ADMIN_ROLE(), rando) is False
     assert token.hasRole(minter_role, rando) is False
     assert token.hasRole(burner_role, rando) is False
+    assert token.hasRole(governor_role, rando) is False
+
+    assert token.getRoleAdmin(
+        token.DEFAULT_ADMIN_ROLE()) == token.DEFAULT_ADMIN_ROLE()
+    assert token.getRoleAdmin(minter_role) == token.DEFAULT_ADMIN_ROLE()
+    assert token.getRoleAdmin(burner_role) == token.DEFAULT_ADMIN_ROLE()
+    assert token.getRoleAdmin(governor_role) == token.DEFAULT_ADMIN_ROLE()
 
 
 def test_erc20(token):

tests/token/test_token_permissions.py

@@ -1,3 +1,5 @@
+from brownie import reverts
+
 
 def test_admin_grant_mint_role_then_revoke(token, admin, rando, minter_role):
     token.grantRole(minter_role, rando, {"from": admin})
@@ -22,3 +24,12 @@ def test_admin_grant_governor_role_then_revoke(token, admin, rando,
 
     token.revokeRole(governor_role, rando, {"from": admin})
     assert token.hasRole(governor_role, rando) is False
+
+
+def test_grant_roles_reverts_when_not_admin(token, rando, minter_role,
+                                            burner_role, governor_role):
+    admin_role = token.DEFAULT_ADMIN_ROLE()
+    roles = [minter_role, burner_role, governor_role, admin_role]
+    for role in roles:
+        with reverts():
+            token.grantRole(role, rando, {"from": rando})