test: invariant basic properties

test/invariant/fuzz/Setup.t.sol

@@ -87,9 +87,7 @@ contract Setup is Actors, Pools {
         for (uint256 i = 1; i <= uint256(type(PoolStrategies).max); i++) {
             address _deployer = _ghost_actors[i % 4];
 
-            IRegistry.Profile memory profile = registry.getProfileByAnchor(
-                _ghost_anchorOf[_deployer]
-            );
+            IRegistry.Profile memory profile = registry.getProfileByAnchor(_ghost_anchorOf[_deployer]);
 
             bytes memory _metadata;
 
@@ -108,9 +106,9 @@ contract Setup is Actors, Pools {
                     token,
                     true
                 );
-            } else if (
-                PoolStrategies(i) == PoolStrategies.EasyRPGF
-            ) {} else if (PoolStrategies(i) == PoolStrategies.ImpactStream) {
+            } else if (PoolStrategies(i) == PoolStrategies.EasyRPGF) {} else if (
+                PoolStrategies(i) == PoolStrategies.ImpactStream
+            ) {
                 _metadata = abi.encode(
                     IRecipientsExtension.RecipientInitializeData({
                         metadataRequired: false,
@@ -164,6 +162,7 @@ contract Setup is Actors, Pools {
             );
 
             ghost_poolAdmins[_poolId] = _deployer;
+            assertTrue(allo.isPoolAdmin(_poolId, _deployer), "Admin not set _initPools_");
 
             _recordPool(_poolId, PoolStrategies(i));
         }

test/invariant/fuzz/handlers/HandlerAllo.t.sol

@@ -3,17 +3,14 @@ pragma solidity ^0.8.19;
 
 import {Setup} from "../Setup.t.sol";
 import {IRegistry} from "contracts/core/Registry.sol";
-import {IAllo, Allo, Metadata} from "contracts/core/Allo.sol";
+import {Allo, IAllo, Metadata} from "contracts/core/Allo.sol";
 import {FuzzERC20} from "../helpers/FuzzERC20.sol";
 
 contract HandlerAllo is Setup {
     mapping(uint256 _poolId => address[] _managers) ghost_poolManagers;
     mapping(uint256 _poolId => address[] _recipients) ghost_recipients;
 
-    function handler_createPool(
-        uint256 _msgValue,
-        uint256 _seedPoolStrategy
-    ) public {
+    function handler_createPool(uint256 _msgValue, uint256 _seedPoolStrategy) public {
         _seedPoolStrategy = bound(
             _seedPoolStrategy,
             uint256(type(PoolStrategies).min) + 1, // Avoid None elt
@@ -27,8 +24,9 @@ contract HandlerAllo is Setup {
         if (profile.anchor == address(0)) return;
 
         // Avoid redeploying pool with a strategy already tested
-        if (_strategyHasImplementation(PoolStrategies(_seedPoolStrategy)))
+        if (_strategyHasImplementation(PoolStrategies(_seedPoolStrategy))) {
             return;
+        }
 
         // Create a pool
         (bool succ, bytes memory ret) = targetCall(
@@ -47,6 +45,13 @@ contract HandlerAllo is Setup {
                 )
             )
         );
+
+        if (succ) {
+            uint256 _poolId = abi.decode(ret, (uint256));
+            ghost_poolIds.push(_poolId);
+            ghost_poolAdmins[_poolId] = msg.sender;
+            assertTrue(allo.isPoolAdmin(_poolId, msg.sender), "Admin not set handler_createPool");
+        }
     }
 
     function handler_updatePoolMetadata(uint256 _idSeed, uint256 _metadataProtocol, string calldata _data) public {
@@ -214,6 +219,7 @@ contract HandlerAllo is Setup {
         (bool success,) = targetCall(address(allo), 0, abi.encodeCall(allo.changeAdmin, (_poolId, _newAdmin)));
         if (success) {
             ghost_poolAdmins[_poolId] = _newAdmin;
+            assertTrue(allo.isPoolAdmin(_poolId, _newAdmin), "Admin not set handler_changeAdmin");
         }
     }
 
@@ -224,7 +230,7 @@ contract HandlerAllo is Setup {
     function _pickPoolId(uint256 _idSeed) internal view returns (uint256) {
         if (ghost_poolIds.length == 0) return 0;
 
-        return ghost_poolIds[_idSeed % ghost_poolIds.length];
+        return ghost_poolIds[_idSeed % ghost_poolIds.length - 1];
     }
 
     function _pickPoolId(uint256[] memory _seeds) internal view returns (uint256[] memory) {
@@ -236,4 +242,13 @@ contract HandlerAllo is Setup {
 
         return _poolIds;
     }
+
+    function _isManager(address _sende, uint256 _poolId) internal returns (bool __isManager) {
+        for (uint256 _i; _i < ghost_poolManagers[_poolId].length; _i++) {
+            if (msg.sender == ghost_poolManagers[_poolId][_i]) {
+                __isManager = true;
+                break;
+            }
+        }
+    }
 }

test/invariant/fuzz/handlers/HandlerStrategy.t.sol

@@ -17,7 +17,7 @@ contract HandlerStrategy is HandlerAllo {
 
         // Withdraw
         (bool succ,) = targetCall(
-            address(allo), 0, abi.encodeCall(strategy_directAllocation.withdraw, (_pool.token, _amount, _recipient))
+            address(_pool.strategy), 0, abi.encodeCall(BaseStrategy.withdraw, (_pool.token, _amount, _recipient))
         );
     }
 }

test/invariant/fuzz/helpers/Actors.t.sol

@@ -117,4 +117,8 @@ contract Actors is Utils {
     function _removeAnchorFromActor(address _actor, bytes32 _profileId) internal {
         delete _ghost_anchorOf[_actor];
     }
+
+    function _pickActor(uint256 _seed) internal view returns (address _actor) {
+        _actor = _ghost_actors[_seed % (_ghost_actors.length - 1)];
+    }
 }

test/invariant/fuzz/helpers/Pools.t.sol

@@ -31,31 +31,17 @@ contract Pools is Utils {
     uint256[] internal ghost_poolIds;
     mapping(uint256 _poolId => address _poolAdmin) ghost_poolAdmins;
 
-    mapping(PoolStrategies _strategy => address _implementation)
-        internal _strategyImplementations;
+    mapping(PoolStrategies _strategy => address _implementation) internal _strategyImplementations;
 
     function _initImplementations(address _allo) internal {
-        _strategyImplementations[PoolStrategies.DirectAllocation] = address(
-            new DirectAllocationStrategy(_allo)
-        );
-        _strategyImplementations[PoolStrategies.DonationVoting] = address(
-            new DonationVotingOnchain(_allo, "MyFancyName")
-        );
-        _strategyImplementations[PoolStrategies.EasyRPGF] = address(
-            new EasyRPGF(_allo)
-        );
-        _strategyImplementations[PoolStrategies.ImpactStream] = address(
-            new QVImpactStream(_allo)
-        );
-        _strategyImplementations[PoolStrategies.QuadraticVoting] = address(
-            new QVSimple(_allo, "MyFancyName")
-        );
-        _strategyImplementations[PoolStrategies.RFP] = address(
-            new RFPSimple(_allo)
-        );
-        _strategyImplementations[PoolStrategies.SQFSuperfluid] = address(
-            new SQFSuperfluid(_allo)
-        );
+        _strategyImplementations[PoolStrategies.DirectAllocation] = address(new DirectAllocationStrategy(_allo));
+        _strategyImplementations[PoolStrategies.DonationVoting] =
+            address(new DonationVotingOnchain(_allo, "MyFancyName"));
+        _strategyImplementations[PoolStrategies.EasyRPGF] = address(new EasyRPGF(_allo));
+        _strategyImplementations[PoolStrategies.ImpactStream] = address(new QVImpactStream(_allo));
+        _strategyImplementations[PoolStrategies.QuadraticVoting] = address(new QVSimple(_allo, "MyFancyName"));
+        _strategyImplementations[PoolStrategies.RFP] = address(new RFPSimple(_allo));
+        _strategyImplementations[PoolStrategies.SQFSuperfluid] = address(new SQFSuperfluid(_allo));
 
         allo = Allo(_allo);
     }
@@ -67,18 +53,14 @@ contract Pools is Utils {
     // reverse lookup pool id -> strategy type
     function _poolStrategy(uint256 _poolId) internal returns (PoolStrategies) {
         IAllo.Pool memory _pool = allo.getPool(_poolId);
-        for (uint256 i; i < uint256(type(PoolStrategies).max); i++)
-            if (
-                _strategyImplementations[PoolStrategies(i)] ==
-                address(_pool.strategy)
-            ) return PoolStrategies(i);
+        for (uint256 i; i < uint256(type(PoolStrategies).max); i++) {
+            if (_strategyImplementations[PoolStrategies(i)] == address(_pool.strategy)) return PoolStrategies(i);
+        }
 
         emit TestFailure("Wrong pool strategy implementation address");
     }
 
-    function _strategyHasImplementation(
-        PoolStrategies _strategy
-    ) internal returns (bool) {
+    function _strategyHasImplementation(PoolStrategies _strategy) internal returns (bool) {
         for (uint256 i; i < ghost_poolIds.length; i++) {
             if (_poolStrategy(ghost_poolIds[i]) == _strategy) return true;
         }

test/invariant/fuzz/properties/PropertiesAllo.t.sol

@@ -25,7 +25,7 @@ contract PropertiesAllo is HandlersParent {
         IRegistry.Profile memory _profile = registry.getProfileByAnchor(_ghost_anchorOf[msg.sender]);
 
         address[] memory _members = new address[](1);
-        address _newMember = _ghost_actors[_actorSeed % (_ghost_actors.length - 1)];
+        address _newMember = _pickActor(_actorSeed);
         _members[0] = _newMember;
 
         (bool _success,) =
@@ -115,27 +115,25 @@ contract PropertiesAllo is HandlersParent {
 
         bytes32 _poolAdminRole = keccak256(abi.encodePacked(_poolId, "admin"));
 
-        address _newAdmin = _ghost_actors[_actorSeed % (_ghost_actors.length - 1)];
+        address _newAdmin = _pickActor(_actorSeed);
 
         (bool _success,) = targetCall(address(allo), 0, abi.encodeCall(allo.changeAdmin, (_poolId, _newAdmin)));
 
-        if (msg.sender == _admin) {
-            if (_success) {
-                if (_newAdmin != _admin) {
-                    assertTrue(
-                        !allo.hasRole(_poolAdminRole, _admin),
-                        "property-id 10: changeAdmin failed remove old admin role not removed"
-                    );
-                }
-                assertTrue(allo.hasRole(_poolAdminRole, _newAdmin), "property-id 10: changeAdmin failed role not set");
-                ghost_poolAdmins[_poolId] = _newAdmin;
-            } else {
-                assertTrue(_newAdmin == address(0) || _usingAnchor, "property-id 10: changeAdmin failed");
+        if (_success) {
+            assertEq(msg.sender, _admin, "property-id 10: changeAdmin only admin should be able to change admin");
+            if (_newAdmin != _admin) {
+                assertTrue(
+                    !allo.hasRole(_poolAdminRole, _admin),
+                    "property-id 10: changeAdmin failed remove old admin role not removed"
+                );
             }
+            assertTrue(allo.hasRole(_poolAdminRole, _newAdmin), "property-id 10: changeAdmin failed role not set");
+            assertTrue(allo.isPoolAdmin(_poolId, _newAdmin), "property-id 10: admin not set");
+            ghost_poolAdmins[_poolId] = _newAdmin;
         } else {
-            if (_success) {
-                fail("property-id 10: changeAdmin only admin should be able to change admin");
-            }
+            assertTrue(
+                _newAdmin == address(0) || msg.sender != _admin || _usingAnchor, "property-id 10: changeAdmin failed"
+            );
         }
     }
 
@@ -153,19 +151,17 @@ contract PropertiesAllo is HandlersParent {
 
         (bool _success,) = targetCall(address(allo), 0, abi.encodeCall(allo.addPoolManagers, (_poolId, _managers)));
 
-        if (msg.sender == _admin) {
-            if (_success) {
-                assertTrue(
-                    allo.hasRole(_poolManagerRole, _newManager), "property-id 11-a: addPoolManagers failed role not set"
-                );
-                ghost_poolManagers[_poolId].push(_newManager);
-            } else {
-                assertTrue(_newManager == address(0) || _usingAnchor, "property-id 11-a: addPoolManager failed");
-            }
+        if (_success) {
+            assertEq(msg.sender, _admin, "property-id 11-a: addPoolManagers only admin should be able to add managers");
+            assertTrue(
+                allo.hasRole(_poolManagerRole, _newManager), "property-id 11-a: addPoolManagers failed role not set"
+            );
+            ghost_poolManagers[_poolId].push(_newManager);
         } else {
-            if (_success) {
-                fail("property-id 11-a: addPoolManager only admin should be able to add managers");
-            }
+            assertTrue(
+                _newManager == address(0) || _usingAnchor || msg.sender != _admin,
+                "property-id 11-a: addPoolManager failed"
+            );
         }
     }
 
@@ -186,23 +182,23 @@ contract PropertiesAllo is HandlersParent {
         (bool _success,) =
             targetCall(address(allo), 0, abi.encodeCall(allo.removePoolManagers, (_poolId, _removeManagers)));
 
-        if (msg.sender == _admin) {
-            if (_success) {
-                assertTrue(!allo.hasRole(_poolManagerRole, _manager), "property-id 11-b: removePoolManagers failed");
-                delete ghost_poolManagers[_poolId];
-                // regenerate the list of managers for the pool
-                for (uint256 _i; _i < _managers.length; _i++) {
-                    if (_i != _managerIndex) {
-                        ghost_poolManagers[_poolId].push(_managers[_i]);
-                    }
+        if (_success) {
+            assertEq(
+                msg.sender, _admin, "property-id 11-b: removePoolManagers only admin should be able to remove managers"
+            );
+            assertTrue(!allo.hasRole(_poolManagerRole, _manager), "property-id 11-b: removePoolManagers failed");
+            delete ghost_poolManagers[_poolId];
+            // regenerate the list of managers for the pool
+            for (uint256 _i; _i < _managers.length; _i++) {
+                if (_i != _managerIndex) {
+                    ghost_poolManagers[_poolId].push(_managers[_i]);
                 }
-            } else {
-                assertTrue(_manager == address(0) || _usingAnchor, "property-id 11-b: removePoolManager failed");
             }
         } else {
-            if (_success) {
-                fail("property-id 11-b: removePoolManager only admin should be able to remove managers");
-            }
+            assertTrue(
+                _manager == address(0) || _usingAnchor || msg.sender != _admin,
+                "property-id 11-b: removePoolManager failed"
+            );
         }
     }
 
@@ -214,31 +210,23 @@ contract PropertiesAllo is HandlersParent {
     function prop_poolManagerCanAlwaysChangeMetadata(uint256 _idSeed, Metadata calldata _metadata) public {
         _idSeed = bound(_idSeed, 0, ghost_poolIds.length - 1);
         uint256 _poolId = ghost_poolIds[_idSeed];
+        address _admin = ghost_poolAdmins[_poolId];
 
         (bool _success,) = targetCall(address(allo), 0, abi.encodeCall(allo.updatePoolMetadata, (_poolId, _metadata)));
 
-        bool _isManager;
-        for (uint256 _i; _i < ghost_poolManagers[_poolId].length; _i++) {
-            if (msg.sender == ghost_poolManagers[_poolId][_i]) {
-                _isManager = true;
-                break;
-            }
-        }
-
-        if (_isManager || msg.sender == ghost_poolAdmins[_poolId]) {
-            if (_success) {
-                Allo.Pool memory _pool = allo.getPool(_poolId);
-                assertEq(
-                    _pool.metadata.protocol, _metadata.protocol, "property-id 13: updatePoolMetadata protocol failed"
-                );
-                assertEq(_pool.metadata.pointer, _metadata.pointer, "property-id 13: updatePoolMetadata pointer failed");
-            } else {
-                assertTrue(_usingAnchor, "property-id 13: updatePoolMetadata failed");
-            }
+        if (_success) {
+            assertTrue(
+                _isManager(msg.sender, _poolId) || msg.sender == _admin,
+                "property-id 13: updatePoolMetadata only manager or admin should be able to update metadata"
+            );
+            Allo.Pool memory _pool = allo.getPool(_poolId);
+            assertEq(_pool.metadata.protocol, _metadata.protocol, "property-id 13: updatePoolMetadata protocol failed");
+            assertEq(_pool.metadata.pointer, _metadata.pointer, "property-id 13: updatePoolMetadata pointer failed");
         } else {
-            if (_success) {
-                fail("property-id 13: updatePoolMetadata only manager should be able to update metadata");
-            }
+            assertTrue(
+                (!_isManager(msg.sender, _poolId) && msg.sender != _admin) || _usingAnchor,
+                "property-id 13: updatePoolMetadata failed"
+            );
         }
     }