diff --git a/test/invariant/PROPERTIES.md b/test/invariant/PROPERTIES.md index 2070cd05..0a02701e 100644 --- a/test/invariant/PROPERTIES.md +++ b/test/invariant/PROPERTIES.md @@ -10,29 +10,19 @@ | 4 | profile owner can always create a pool | | 5 | anyone can increase fund in a pool, if strategy (hook) logic allows so and if more than base fee | | 6 | every deposit/pool creation must take the correct fee on the amount deposited, forwarded to the treasury | -| ACC-1 | Balance sheet is balanced, no bad debt | +| ACC-1 | There is no token which has left the protocol without being accounted for | | ACC-2 | Each pool is solvable | -The last 2 invariants are general procotol accounting, based on the following balance sheet: +The last 2 invariants are general procotol accounting, one based on the actual overal "cash-flow" and one based on the individual pool accounting. Protocol balance sheet: -| asset | liabilities | -| ---------------------------------- | ---------------------------------- | -| unallocated tokens in strategies | fund available to allocate | -| (unaccounted tokens in strategies) | tokens allocated but not withdrawn | - -The following operations are covered by this accounting -bookholding writings (allocate should be a simple reclassification, kept it as a double-writing for clarity) -- createPool, fundPool: credit: unallocated tokens in strategies, debit: fund available to allocate for a pool -- allocate, batchAllocate: credit: tokens allocated but not withdrawn, debit: unallocated tokens in strategies -- distribute: credit: token sent to recipient, debit: tokens allocated but not withdrawn -- direct transfer to pool: credit: unaccounted tokens in strategies, debit: fund received -- withdraw: credit: token sent to poolOwner, debit: unaccounted tokens in strategies - -These 2 are omitted, as they're covered by unit tests: -- direct transfer to protocol contract: credit: tokens in core protocol contract, debit: fund received -- recoverFunds: credit: token send to protocol owner, debit: tokens in core protocol contract +| asset | liabilities | +| ---------------------------------- | ------------------------------------ | +| tokens in strategies | withdrawable tokens (unallocated) | +| (unaccounted tokens in strategies) | tokens to distribute | +Note: Allocation doesn't imply token movement. Instead, it set the *future* balance sheet movements, +which will effectively occur when distributing or withdrawing tokens (accounting wise, it's a requalification of liabilities). ## Other important invariant, covered by the unit tests diff --git a/test/invariant/fuzz/FoundryReproducer.t.sol b/test/invariant/fuzz/FoundryReproducer.t.sol index dcf96502..bf8605c3 100644 --- a/test/invariant/fuzz/FoundryReproducer.t.sol +++ b/test/invariant/fuzz/FoundryReproducer.t.sol @@ -5,7 +5,5 @@ import {FuzzTest} from "./FuzzTest.t.sol"; import {Actors} from "./Setup.t.sol"; contract FoundryReproducer is FuzzTest { - function test_reproduce() public { - assert(true); - } + function test_reproduce() public {} } diff --git a/test/invariant/fuzz/handlers/HandlerStrategy.t.sol b/test/invariant/fuzz/handlers/HandlerStrategy.t.sol index 0abff002..ea48addd 100644 --- a/test/invariant/fuzz/handlers/HandlerStrategy.t.sol +++ b/test/invariant/fuzz/handlers/HandlerStrategy.t.sol @@ -14,22 +14,7 @@ contract HandlerStrategy is HandlerAllo { IAllo.Pool memory _pool = allo.getPool(ghost_poolIds[_poolSeed]); ERC20(_pool.token).transfer(address(_pool.strategy), _amount); - } - - function handler_withdraw(uint256 _poolSeed, uint256 _amount) public { - address _recipient = makeAddr("IAmRecipient"); - - // Needs at least one pool - if (ghost_poolIds.length == 0) return; - - // Get the pool - _poolSeed = _poolSeed % ghost_poolIds.length; - IAllo.Pool memory _pool = allo.getPool(ghost_poolIds[_poolSeed]); - // Withdraw - Actors _actor = _currentActor(); - (bool succ,) = _actor.callThroughAnchor( - address(_pool.strategy), 0, abi.encodeCall(BaseStrategy.withdraw, (_pool.token, _amount, _recipient)) - ); + ghost_totalReceived += _amount; } } diff --git a/test/invariant/fuzz/helpers/Actors.t.sol b/test/invariant/fuzz/helpers/Actors.t.sol index 073f2ec1..76da00ed 100644 --- a/test/invariant/fuzz/helpers/Actors.t.sol +++ b/test/invariant/fuzz/helpers/Actors.t.sol @@ -71,9 +71,7 @@ contract Actors is Utils { contract HandlerActors is Utils, GhostStorage { function _currentActor() internal view returns (Actors _actor) { uint256 _seed = uint256(uint160(msg.sender)); - _actor = Actors( - payable(_ghost_actors[(_seed % _ghost_actors.length) - 1]) - ); + _actor = Actors(payable(_ghost_actors[(_seed % _ghost_actors.length)])); } function _randomActor(uint256 _seed) internal view returns (Actors _actor) { diff --git a/test/invariant/fuzz/helpers/GhostStorage.t.sol b/test/invariant/fuzz/helpers/GhostStorage.t.sol index c93cab61..9ac1b859 100644 --- a/test/invariant/fuzz/helpers/GhostStorage.t.sol +++ b/test/invariant/fuzz/helpers/GhostStorage.t.sol @@ -10,9 +10,7 @@ contract GhostStorage { mapping(uint256 _poolId => address[] _recipients) ghost_recipients; - uint256 ghost_totalReceived; // only net amounts, after fee (direct transfer) - uint256 ghost_availableToAllocate; - uint256 ghost_totalAllocatedNotDistributed; + uint256 ghost_totalReceived; // only net amounts, after fee (which is directly transfered) uint256 ghost_totalWithdrawn; mapping(uint256 _poolId => mapping(address _owner => uint256 _amount)) ghost_allocations; diff --git a/test/invariant/fuzz/helpers/Pools.t.sol b/test/invariant/fuzz/helpers/Pools.t.sol index a687b133..974415d3 100644 --- a/test/invariant/fuzz/helpers/Pools.t.sol +++ b/test/invariant/fuzz/helpers/Pools.t.sol @@ -65,16 +65,18 @@ contract Pools is Utils, GhostStorage { // // Getters // + event AggregatePoolBalances(uint256); + function _aggregatePoolBalances() internal returns (uint256 _aggregateBalance) { - // Cumulative sum based on the pool internal balances, we're assessing - // its accuracy in a property (avoid "hiding" bad debt in unaccounted - // tokens) uint256 _totalPoolBalances; for (uint256 i; i < ghost_poolIds.length; i++) { uint256 poolId = ghost_poolIds[i]; - _totalPoolBalances += IBaseStrategy(allo.getPool(poolId).strategy).getPoolAmount(); + IERC20 token = IERC20(allo.getPool(poolId).token); + + _totalPoolBalances += token.balanceOf(address(allo.getPool(poolId).strategy)); } + emit AggregatePoolBalances(_totalPoolBalances); return _totalPoolBalances; } diff --git a/test/invariant/fuzz/properties/PropertiesAllo.t.sol b/test/invariant/fuzz/properties/PropertiesAllo.t.sol index 2761adf3..ca103d4e 100644 --- a/test/invariant/fuzz/properties/PropertiesAllo.t.sol +++ b/test/invariant/fuzz/properties/PropertiesAllo.t.sol @@ -109,7 +109,6 @@ contract PropertiesAllo is HandlersParent { ); ghost_totalReceived += _amountAfterFee; - ghost_availableToAllocate += _amountAfterFee; } else { // Edge-case of some strategies: only allocation during a defined period ( diff --git a/test/invariant/fuzz/properties/PropertiesStrategies.t.sol b/test/invariant/fuzz/properties/PropertiesStrategies.t.sol index 7c26c160..d91931b4 100644 --- a/test/invariant/fuzz/properties/PropertiesStrategies.t.sol +++ b/test/invariant/fuzz/properties/PropertiesStrategies.t.sol @@ -8,7 +8,9 @@ import {IAllo, Allo, Metadata} from "contracts/core/Allo.sol"; import {IRegistry, Registry} from "contracts/core/Registry.sol"; import {IBaseStrategy} from "contracts/strategies/BaseStrategy.sol"; import {IAllocationExtension} from "contracts/strategies/extensions/allocate/IAllocationExtension.sol"; -import {RecipientsExtension, IRecipientsExtension} from "contracts/strategies/extensions/register/RecipientsExtension.sol"; +import { + RecipientsExtension, IRecipientsExtension +} from "contracts/strategies/extensions/register/RecipientsExtension.sol"; import {IAllocatorsAllowlistExtension} from "contracts/strategies/extensions/allocate/IAllocatorsAllowlistExtension.sol"; import {Errors} from "contracts/core/libraries/Errors.sol"; @@ -19,14 +21,11 @@ import {Actors} from "../helpers/Actors.t.sol"; contract PropertiesStrategies is HandlersParent { ///@custom:property-id ACC-1 - ///@custom:property Balance sheet is balanced / no bad debt - function property_checkBalanceSheet() public { + ///@custom:property There is no token which has left the protocol without being accounted for + function property_checkNoTokenOutUnaccounted() public { assertTrue( - ghost_totalReceived == - ghost_availableToAllocate + - ghost_totalAllocatedNotDistributed + - ghost_totalWithdrawn, - "Accounting: balance sheet inbalance" + ghost_totalReceived == _aggregatePoolBalances() + ghost_totalWithdrawn, + "Accounting: token out unaccounted for" ); } @@ -35,30 +34,18 @@ contract PropertiesStrategies is HandlersParent { function property_checkPoolSolvability() public { for (uint256 i; i < ghost_poolIds.length; i++) { uint256 poolId = ghost_poolIds[i]; - uint256 poolReportedBalance = allo - .getPool(poolId) - .strategy - .getPoolAmount(); + uint256 poolReportedBalance = allo.getPool(poolId).strategy.getPoolAmount(); - uint256 poolObservedBalance = token.balanceOf( - address(allo.getPool(poolId).strategy) - ); + uint256 poolObservedBalance = token.balanceOf(address(allo.getPool(poolId).strategy)); // gte as we have a handler for token direct transfer to the pool - assertTrue( - poolObservedBalance >= poolReportedBalance, - "Accounting: pool internal insolvability" - ); + assertTrue(poolObservedBalance >= poolReportedBalance, "Accounting: pool internal insolvability"); } } ///@custom:property-id 1 ///@custom:property manager should be able to allocate for recipient (based on strategy) - function prop_userShouldBeAbleToAllocateForRecipient( - uint256 _actorSeed, - uint256 _idSeed, - uint256 _amount - ) public { + function prop_userShouldBeAbleToAllocateForRecipient(uint256 _actorSeed, uint256 _idSeed, uint256 _amount) public { Actors _actor = _currentActor(); address _allocator = _currentActor().controlledAnchor(); @@ -80,49 +67,29 @@ contract PropertiesStrategies is HandlersParent { address _strategy = address(allo.getPool(_poolId).strategy); // DV needs the token in _data - bytes memory _data = _poolStrategy(_strategy) == - PoolStrategies.DonationVoting + bytes memory _data = _poolStrategy(_strategy) == PoolStrategies.DonationVoting ? abi.encode(token, new bytes(0)) : abi.encode(_tokens); // Needed for direct allocation token.transfer(_allocator, _amount); - _actor.callThroughAnchor( - address(token), - 0, - abi.encodeCall(ERC20.approve, (address(_strategy), _amount)) - ); + _actor.callThroughAnchor(address(token), 0, abi.encodeCall(ERC20.approve, (address(_strategy), _amount))); (bool _success, bytes memory _ret) = _actor.callThroughAnchor( - address(allo), - 0, - abi.encodeCall( - allo.allocate, - (_poolId, _recipients, _amounts, _data) - ) + address(allo), 0, abi.encodeCall(allo.allocate, (_poolId, _recipients, _amounts, _data)) ); if (_success) { // Check strategy specific post-conditions - _assertValidAllocate( - payable(_strategy), - _allocator, - _recipient, - _amount - ); + _assertValidAllocate(payable(_strategy), _allocator, _recipient, _amount); - ghost_totalAllocatedNotDistributed += _amount; - - if (_poolStrategy(_strategy) != PoolStrategies.DirectAllocation) { - ghost_allocations[_poolId][address(_recipient)] += _amount; - } else { + if (_poolStrategy(_strategy) == PoolStrategies.DirectAllocation) { // allocation is directly distributed - ghost_totalAllocatedNotDistributed -= _amount; + ghost_totalReceived += _amount; ghost_totalWithdrawn += _amount; + } else { + ghost_allocations[_poolId][address(_recipient)] += _amount; } - - ghost_availableToAllocate -= _amount; - ghost_allocations[_poolId][_recipient] += _amount; } else { _assertInvalidAllocate(_strategy, _allocator, _ret); } @@ -145,9 +112,7 @@ contract PropertiesStrategies is HandlersParent { _idSeed = bound(_idSeed, 0, ghost_poolIds.length - 1); uint256 _poolId = ghost_poolIds[_idSeed]; - address _manager = ghost_poolManagers[_poolId][ - (_managerSeed % ghost_poolManagers[_poolId].length) - ]; + address _manager = ghost_poolManagers[_poolId][(_managerSeed % ghost_poolManagers[_poolId].length)]; IBaseStrategy _strategy = allo.getPool(_poolId).strategy; @@ -156,21 +121,16 @@ contract PropertiesStrategies is HandlersParent { uint256 _poolAmount = _strategy.getPoolAmount(); vm.prank(_manager); - (bool _success, bytes memory _ret) = address(allo).call( - abi.encodeCall(allo.distribute, (_poolId, _recipients, _data)) - ); + (bool _success, bytes memory _ret) = + address(allo).call(abi.encodeCall(allo.distribute, (_poolId, _recipients, _data))); // General (non-)revertion assertion, should be common to all strategies if (_success) { uint256 _recipientNewBalance = token.balanceOf(_recipient); // Allocation if there is one - assertTrue( - _hasAllocation, - "property-id 3: Distribution succeeded without allocation" - ); + assertTrue(_hasAllocation, "property-id 3: Distribution succeeded without allocation"); - ghost_totalAllocatedNotDistributed -= _amount; ghost_totalWithdrawn += _amount; } else { // Revert if: @@ -178,10 +138,8 @@ contract PropertiesStrategies is HandlersParent { // - There is not enough token to cover the allocation // - allocate() is not implemented by the strategy assertTrue( - !_hasAllocation || - _totalAllocatedInPool(_poolId) > _poolAmount || - abi.decode(_ret, (bytes4)) == - Errors.NOT_IMPLEMENTED.selector, + !_hasAllocation || _totalAllocatedInPool(_poolId) > _poolAmount + || bytes4(_ret) == Errors.NOT_IMPLEMENTED.selector, "property-id 3: Distribution failed while allocation should be valid" ); } @@ -189,46 +147,27 @@ contract PropertiesStrategies is HandlersParent { ///@custom:property-id 3 ///@custom:property only funds outside the poolAmount can be withdrawn from a pool - function prop_onlyUnallocatedWithdrawable( - uint256 _poolSeed, - uint256 _amount - ) public { + function prop_onlyUnallocatedWithdrawable(uint256 _poolSeed, uint256 _amount) public { uint256 _poolId = _pickPoolId(_poolSeed); address _strategy = allo.getStrategy(_poolId); address _manager = ghost_poolManagers[_poolId][0]; uint256 poolActualBalance = token.balanceOf(_strategy); - uint256 poolInternalBalance = allo - .getPool(_poolId) - .strategy - .getPoolAmount(); + uint256 poolInternalBalance = allo.getPool(_poolId).strategy.getPoolAmount(); Actors _actor = _currentActor(); - (bool success, ) = _actor.callThroughAnchor( - address(_strategy), - 0, - abi.encodeCall( - IBaseStrategy.withdraw, - (address(token), _amount, msg.sender) - ) + (bool success,) = _actor.callThroughAnchor( + address(_strategy), 0, abi.encodeCall(IBaseStrategy.withdraw, (address(token), _amount, msg.sender)) ); if (success) { - assertTrue( - _actor.controlledAnchor() == _manager, - "property-id 3: Caller not manager" - ); + assertTrue(_actor.controlledAnchor() == _manager, "property-id 3: Caller not manager"); - assertTrue( - _amount <= poolActualBalance - poolInternalBalance, - "property-id 3: Withdrew allocated funds" - ); + assertTrue(_amount <= poolActualBalance - poolInternalBalance, "property-id 3: Withdrew allocated funds"); + + ghost_totalWithdrawn += _amount; } else { - assertTrue( - _actor.controlledAnchor() != _manager || - _amount > poolActualBalance - poolInternalBalance, - "property-id 3: Withdraw failed" - ); + // Strategy-specific revert conditions } } @@ -237,103 +176,75 @@ contract PropertiesStrategies is HandlersParent { // // Check strategy dependent post-conditions if a call to allocate is successful - function _assertValidAllocate( - address payable _strategy, - address _allocator, - address _recipient, - uint256 _amount - ) internal { + function _assertValidAllocate(address payable _strategy, address _allocator, address _recipient, uint256 _amount) + internal + { if ( - _poolStrategy(_strategy) == PoolStrategies.QuadraticVoting || - _poolStrategy(_strategy) == PoolStrategies.ImpactStream + _poolStrategy(_strategy) == PoolStrategies.QuadraticVoting + || _poolStrategy(_strategy) == PoolStrategies.ImpactStream ) { assertTrue( - IAllocatorsAllowlistExtension(address(_strategy)) - .allowedAllocators(_allocator), + IAllocatorsAllowlistExtension(address(_strategy)).allowedAllocators(_allocator), "property-id 1 QV/IS: allocator not allowed" ); } else if (_poolStrategy(_strategy) == PoolStrategies.DonationVoting) { assertTrue( - IAllocationExtension(_strategy).allocationStartTime() <= - block.timestamp && - IAllocationExtension(_strategy).allocationEndTime() >= - block.timestamp, + IAllocationExtension(_strategy).allocationStartTime() <= block.timestamp + && IAllocationExtension(_strategy).allocationEndTime() >= block.timestamp, "property-id 1 DV: allocate outside of allocation window" ); - } else if ( - _poolStrategy(_strategy) == PoolStrategies.FuzzBaseStrategy - ) { + } else if (_poolStrategy(_strategy) == PoolStrategies.FuzzBaseStrategy) { assertTrue( FuzzBaseStrategy(_strategy).allocated(_recipient) == _amount, "property-id 1 Fuzz: wrong amount allocated" ); - } else if ( - _poolStrategy(_strategy) == PoolStrategies.DirectAllocation - ) { + } else if (_poolStrategy(_strategy) == PoolStrategies.DirectAllocation) { // Empty } else { - fail( - "property-id 1: allocate call succeeded but should have failed" - ); + fail("property-id 1: allocate call succeeded but should have failed"); } } - function _assertInvalidAllocate( - address _strategy, - address _allocator, - bytes memory _ret - ) internal { + function _assertInvalidAllocate(address _strategy, address _allocator, bytes memory _ret) internal { if ( - _poolStrategy(_strategy) == PoolStrategies.QuadraticVoting || - _poolStrategy(_strategy) == PoolStrategies.ImpactStream + _poolStrategy(_strategy) == PoolStrategies.QuadraticVoting + || _poolStrategy(_strategy) == PoolStrategies.ImpactStream ) { assertFalse( - IAllocatorsAllowlistExtension(address(_strategy)) - .allowedAllocators(_allocator), + IAllocatorsAllowlistExtension(address(_strategy)).allowedAllocators(_allocator), "property-id 2 QV/IS: allocator allowed but failed" ); } else if ( - _poolStrategy(_strategy) == PoolStrategies.RFP || - _poolStrategy(_strategy) == PoolStrategies.EasyRPGF - ) { - assertEq( - abi.decode(_ret, (bytes4)), - bytes4(Errors.NOT_IMPLEMENTED.selector), - "property-id 2 RFP/RPGF: wrong allocate() revert" - ); - } - // allocate not implemented - else if (_poolStrategy(_strategy) == PoolStrategies.DonationVoting) { - bytes4 _error = abi.decode(_ret, (bytes4)); - - // Getter for recipient status is not implemented (yet?) - if ( - abi.decode(_ret, (bytes4)) != - bytes4( - IRecipientsExtension - .RecipientsExtension_RecipientNotAccepted - .selector - ) - ) { - assertTrue( - IAllocationExtension(_strategy).allocationStartTime() > - block.timestamp || - IAllocationExtension(_strategy).allocationEndTime() < - block.timestamp, - "property 2 DV: Allocation failed in correct period" - ); - } - } else if ( - _poolStrategy(_strategy) == PoolStrategies.FuzzBaseStrategy + _poolStrategy(_strategy) == PoolStrategies.RFP || _poolStrategy(_strategy) == PoolStrategies.EasyRPGF ) { + // Not implemented + } else if (_poolStrategy(_strategy) == PoolStrategies.DonationVoting) { + // Todo: anchor doesn't bubble the error up, this check is catching any "CALL_FAILED()" + // and recipient extension has no getter for the recipient status + // if ( + // bytes4(_ret) != + // IRecipientsExtension + // .RecipientsExtension_RecipientNotAccepted + // .selector + // ) { + // // Revert if: outside of allocation window, allocator not allowed, wrong recipient status + // assertTrue( + // IAllocationExtension(_strategy).allocationStartTime() > + // block.timestamp || + // IAllocationExtension(_strategy).allocationEndTime() < + // block.timestamp || + // !IAllocatorsAllowlistExtension(address(_strategy)) + // .allowedAllocators(_allocator), + // "property 2 DV: Allocation failed in correct period" + // ); + // } + } else if (_poolStrategy(_strategy) == PoolStrategies.FuzzBaseStrategy) { assertTrue( !_isManager(_allocator, IBaseStrategy(_strategy).getPoolId()), "property-id 2: wrong allocate() revert for FuzzBaseStrategy" ); } else { - fail( - "property-id 2: allocate call failed but should have succeeded" - ); + fail("property-id 2: allocate call failed but should have succeeded"); } } }