From 1fee011b734fa1b2b50bf8c30b9dc1cdf4d9917b Mon Sep 17 00:00:00 2001 From: gallo Date: Fri, 11 Oct 2024 16:27:48 +0200 Subject: [PATCH] feat: return 0 on duplicate claim --- INTEGRATION.MD | 8 ++++++++ src/Governance.sol | 15 ++++++++++----- test/Governance.t.sol | 21 +++++---------------- 3 files changed, 23 insertions(+), 21 deletions(-) create mode 100644 INTEGRATION.MD diff --git a/INTEGRATION.MD b/INTEGRATION.MD new file mode 100644 index 00000000..f2ce9d2d --- /dev/null +++ b/INTEGRATION.MD @@ -0,0 +1,8 @@ +# Risks to integrators + +Somebody could claim on your behalf + +Votes not meeting the threshold may result in 0 rewards + +Claiming more than once will return 0 + diff --git a/src/Governance.sol b/src/Governance.sol index b2a4b52e..dd4da503 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -581,13 +581,18 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance (VoteSnapshot memory votesSnapshot_,) = _snapshotVotes(); (InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState_) = _snapshotVotesForInitiative(_initiative); - /// Invariant: Must only claim once or unregister - require(initiativeState_.lastEpochClaim < epoch() - 1, "Governance: already-claimed"); /// TODO: Merge into rest - // TODO: We can do a early return instead - // TODO: Return type from state FSM can be standardized (, bool canClaimRewards, ) = getInitiativeState(_initiative); - require(canClaimRewards, "Governance: claim-not-met"); + + /// @audit Return 0 if we cannot claim + /// INVARIANT: + /// We cannot claim only for 2 reasons: + /// We have already claimed + /// We do not meet the threshold + /// TODO: Enforce this with assertions + if(!canClaimRewards) { + return 0; + } // if(!canClaimRewards) { // return 0; // } diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 9e70832c..b2245693 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -1126,14 +1126,11 @@ contract GovernanceTest is Test { // should compute the claim and transfer it to the initiative assertEq(governance.claimForInitiative(baseInitiative1), 5000e18); - - vm.expectRevert("Governance: already-claimed"); /// @audit TODO: Decide if we should not revert - governance.claimForInitiative(baseInitiative1); + // 2nd claim = 0 + assertEq(governance.claimForInitiative(baseInitiative1), 0); assertEq(governance.claimForInitiative(baseInitiative2), 5000e18); - - vm.expectRevert("Governance: already-claimed"); - governance.claimForInitiative(baseInitiative2); + assertEq(governance.claimForInitiative(baseInitiative2), 0); assertEq(lusd.balanceOf(baseInitiative2), 5000e18); @@ -1154,21 +1151,13 @@ contract GovernanceTest is Test { vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1); assertEq(governance.claimForInitiative(baseInitiative1), 10000e18); + assertEq(governance.claimForInitiative(baseInitiative1), 0); - vm.expectRevert("Governance: already-claimed"); - governance.claimForInitiative(baseInitiative1); assertEq(lusd.balanceOf(baseInitiative1), 15000e18); - // TODO: This should most likely either return 0 or we accept the claim not met - /// Claim not met is kind of a weird thing to return tbh - /// @audit THIS FAILS ON PURPOSE - /// TODO: Let's fix this by fixing single claiming - /// Let's decide how to handle 0 rewards case and then decide assertEq(governance.claimForInitiative(baseInitiative2), 0); - - vm.expectRevert("Governance: already-claimed"); - governance.claimForInitiative(baseInitiative2); + assertEq(governance.claimForInitiative(baseInitiative2), 0); assertEq(lusd.balanceOf(baseInitiative2), 5000e18);