Skip to content

Commit

Permalink
feat: return 0 on duplicate claim
Browse files Browse the repository at this point in the history
  • Loading branch information
GalloDaSballo committed Oct 11, 2024
1 parent 2c52bc3 commit 1fee011
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 21 deletions.
8 changes: 8 additions & 0 deletions INTEGRATION.MD
Original file line number Diff line number Diff line change
@@ -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

15 changes: 10 additions & 5 deletions src/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
// }
Expand Down
21 changes: 5 additions & 16 deletions test/Governance.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand Down

0 comments on commit 1fee011

Please sign in to comment.