From dce577ac157f61817db5fbb267fc0e7e1559b049 Mon Sep 17 00:00:00 2001 From: Paul Razvan Berg Date: Tue, 28 Nov 2023 11:42:05 +0200 Subject: [PATCH] refactor: move protocol fee check at the bottom --- src/abstracts/SablierV2MerkleStreamer.sol | 10 ++--- src/interfaces/ISablierV2MerkleStreamer.sol | 5 ++- .../merkle-streamer/ll/claim/claim.t.sol | 41 +++++++++++-------- .../merkle-streamer/ll/claim/claim.tree | 26 ++++++------ 4 files changed, 46 insertions(+), 36 deletions(-) diff --git a/src/abstracts/SablierV2MerkleStreamer.sol b/src/abstracts/SablierV2MerkleStreamer.sol index 2b8f29fe..c8d1cdd8 100644 --- a/src/abstracts/SablierV2MerkleStreamer.sol +++ b/src/abstracts/SablierV2MerkleStreamer.sol @@ -131,6 +131,11 @@ abstract contract SablierV2MerkleStreamer is revert Errors.SablierV2MerkleStreamer_StreamClaimed(index); } + // Checks: the input claim is included in the Merkle tree. + if (!MerkleProof.verify(merkleProof, MERKLE_ROOT, leaf)) { + revert Errors.SablierV2MerkleStreamer_InvalidProof(); + } + // Safe Interactions: query the protocol fee. This is safe because it's a known Sablier contract that does // not call other unknown contracts. UD60x18 protocolFee = LOCKUP.comptroller().protocolFees(ASSET); @@ -139,10 +144,5 @@ abstract contract SablierV2MerkleStreamer is if (protocolFee.gt(ud(0))) { revert Errors.SablierV2MerkleStreamer_ProtocolFeeNotZero(); } - - // Checks: the input claim is included in the Merkle tree. - if (!MerkleProof.verify(merkleProof, MERKLE_ROOT, leaf)) { - revert Errors.SablierV2MerkleStreamer_InvalidProof(); - } } } diff --git a/src/interfaces/ISablierV2MerkleStreamer.sol b/src/interfaces/ISablierV2MerkleStreamer.sol index 28b6c1db..42dc7a4d 100644 --- a/src/interfaces/ISablierV2MerkleStreamer.sol +++ b/src/interfaces/ISablierV2MerkleStreamer.sol @@ -66,9 +66,12 @@ interface ISablierV2MerkleStreamer is IAdminable { /// /// @dev Emits a {Clawback} event. /// + /// Notes: + /// - If the protocol is not zero, the expiration check is not made. + /// /// Requirements: /// - The caller must be the admin. - /// - The Merkle streamer must have expired. + /// - The campaign must either be expired or not have an expiration. /// /// @param to The address to receive the tokens. /// @param amount The amount of tokens to claw back. diff --git a/test/integration/merkle-streamer/ll/claim/claim.t.sol b/test/integration/merkle-streamer/ll/claim/claim.t.sol index 1ceaeaf8..1390696a 100644 --- a/test/integration/merkle-streamer/ll/claim/claim.t.sol +++ b/test/integration/merkle-streamer/ll/claim/claim.t.sol @@ -41,18 +41,6 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { _; } - function test_RevertGiven_ProtocolFeeNotZero() external givenCampaignNotExpired givenNotClaimed { - bytes32[] memory merkleProof; - changePrank({ msgSender: users.admin.addr }); - comptroller.setProtocolFee({ asset: asset, newProtocolFee: ud(0.03e18) }); - vm.expectRevert(Errors.SablierV2MerkleStreamer_ProtocolFeeNotZero.selector); - merkleStreamerLL.claim({ index: 1, recipient: users.recipient1.addr, amount: 1, merkleProof: merkleProof }); - } - - modifier givenProtocolFeeZero() { - _; - } - modifier givenNotIncludedInMerkleTree() { _; } @@ -61,7 +49,6 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { external givenCampaignNotExpired givenNotClaimed - givenProtocolFeeZero givenNotIncludedInMerkleTree { uint256 invalidIndex = 1337; @@ -75,7 +62,6 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { external givenCampaignNotExpired givenNotClaimed - givenProtocolFeeZero givenNotIncludedInMerkleTree { uint256 index1 = defaults.INDEX1(); @@ -90,7 +76,6 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { external givenCampaignNotExpired givenNotClaimed - givenProtocolFeeZero givenNotIncludedInMerkleTree { uint256 index1 = defaults.INDEX1(); @@ -104,7 +89,6 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { external givenCampaignNotExpired givenNotClaimed - givenProtocolFeeZero givenNotIncludedInMerkleTree { uint256 index1 = defaults.INDEX1(); @@ -118,12 +102,35 @@ contract Claim_Integration_Test is MerkleStreamer_Integration_Test { _; } + function test_RevertGiven_ProtocolFeeNotZero() + external + givenCampaignNotExpired + givenNotClaimed + givenIncludedInMerkleTree + { + uint128 claimAmount = defaults.CLAIM_AMOUNT(); + bytes32[] memory merkleProof = defaults.index1Proof(); + changePrank({ msgSender: users.admin.addr }); + comptroller.setProtocolFee({ asset: asset, newProtocolFee: ud(0.1e18) }); + vm.expectRevert(Errors.SablierV2MerkleStreamer_ProtocolFeeNotZero.selector); + merkleStreamerLL.claim({ + index: 1, + recipient: users.recipient1.addr, + amount: claimAmount, + merkleProof: merkleProof + }); + } + + modifier givenProtocolFeeZero() { + _; + } + function test_Claim() external givenCampaignNotExpired givenNotClaimed - givenProtocolFeeZero givenIncludedInMerkleTree + givenProtocolFeeZero { uint256 expectedStreamId = lockupLinear.nextStreamId(); diff --git a/test/integration/merkle-streamer/ll/claim/claim.tree b/test/integration/merkle-streamer/ll/claim/claim.tree index b145b2f6..d59287b0 100644 --- a/test/integration/merkle-streamer/ll/claim/claim.tree +++ b/test/integration/merkle-streamer/ll/claim/claim.tree @@ -5,19 +5,19 @@ claim.t.sol ├── given the recipient has claimed │ └── it should revert └── given the recipient has not claimed - ├── given the protocol fee is greater than zero - │ └── it should revert - └── given the protocol fee is not greater than zero - ├── given the claim is not included in the Merkle tree - │ ├── when the index is not valid - │ │ └── it should revert - │ ├── when the recipient address is not valid - │ │ └── it should revert - │ ├── when the amount is not valid - │ │ └── it should revert - │ └── when the Merkle proof is not valid - │ └── it should revert - └── given the claim is included in the Merkle tree + ├── given the claim is not included in the Merkle tree + │ ├── when the index is not valid + │ │ └── it should revert + │ ├── when the recipient address is not valid + │ │ └── it should revert + │ ├── when the amount is not valid + │ │ └── it should revert + │ └── when the Merkle proof is not valid + │ └── it should revert + └── given the claim is included in the Merkle tree + ├── given the protocol fee is greater than zero + │ └── it should revert + └── given the protocol fee is not greater than zero ├── it should mark the index as claimed ├── it should create a LockupLinear stream └── it should emit a {Claim} event