Skip to content

Commit 0d99905

Browse files
bengobeilbengobeil
authored
fix: validate payClaim in msg.value (#96)
1 parent 63f9227 commit 0d99905

4 files changed

Lines changed: 298 additions & 0 deletions

File tree

‎src/BullaClaimV2.sol‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,13 @@ contract BullaClaimV2 is ERC721, Ownable, IBullaClaimV2 {
236236
// isn't trying to bypass controller specific logic (eg: late fees) and by going to this contract directly.
237237
if (claim.controller != address(0) && msg.sender != claim.controller) revert NotController(msg.sender);
238238

239+
// Validate msg.value for ETH payments to prevent underpayment vulnerability
240+
if (claim.token == address(0)) {
241+
if (msg.value != paymentAmount) {
242+
revert IncorrectMsgValue();
243+
}
244+
}
245+
239246
// Update payment state first to follow checks-effects-interactions pattern
240247
_updateClaimPaymentState(from, claimId, claim, paymentAmount);
241248

‎src/interfaces/IBullaClaimV2.sol‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ interface IBullaClaimV2 is IBullaClaimCore, IBullaClaimAdmin {
3030
error ApprovalExpired();
3131
error NotSupported();
3232
error MustBeControlledClaim();
33+
error IncorrectMsgValue();
3334

3435
/*///////////////////////////////////////////////////////////////
3536
EVENTS

‎test/foundry/BullaClaim/PayClaim/PayClaimInsufficientEth.t.sol‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
// SPDX-License-Identifier: GPL-2.0-or-later
2+
pragma solidity ^0.8.30;
3+
4+
import "forge-std/Vm.sol";
5+
import "forge-std/Test.sol";
6+
import "forge-std/console.sol";
7+
import "contracts/types/Types.sol";
8+
import {BullaClaimV2} from "contracts/BullaClaimV2.sol";
9+
import {IBullaClaimV2} from "contracts/interfaces/IBullaClaimV2.sol";
10+
import {BullaClaimTestHelper} from "test/foundry/BullaClaim/BullaClaimTestHelper.sol";
11+
import {DeployContracts} from "script/DeployContracts.s.sol";
12+
import {CreateClaimParamsBuilder} from "test/foundry/BullaClaim/CreateClaimParamsBuilder.sol";
13+
14+
/**
15+
* @title TestPayClaimInsufficientEth
16+
* @notice Tests to validate ETH payment validation in BullaClaimV2
17+
* @dev These tests verify that BullaClaimV2 now correctly validates msg.value == paymentAmount
18+
* matching the secure behavior of BullaInvoice
19+
*/
20+
contract TestPayClaimInsufficientEth is BullaClaimTestHelper {
21+
address creditor = address(0xA11c3);
22+
address debtor = address(0xB0b);
23+
24+
function setUp() public {
25+
vm.label(address(this), "TEST_CONTRACT");
26+
vm.label(creditor, "CREDITOR");
27+
vm.label(debtor, "DEBTOR");
28+
29+
DeployContracts.DeploymentResult memory deploymentResult =
30+
(new DeployContracts()).deployForTest(address(this), LockState.Unlocked, 0, 0, 0, address(this));
31+
bullaClaim = BullaClaimV2(deploymentResult.bullaClaim);
32+
approvalRegistry = bullaClaim.approvalRegistry();
33+
34+
// Give debtor and creditor ETH
35+
vm.deal(creditor, 1000 ether);
36+
vm.deal(debtor, 1000 ether);
37+
}
38+
39+
function _createNativeEthClaim(uint256 claimAmount) private returns (uint256 claimId) {
40+
vm.startPrank(creditor);
41+
claimId = bullaClaim.createClaim(
42+
new CreateClaimParamsBuilder().withCreditor(creditor).withDebtor(debtor).withClaimAmount(claimAmount)
43+
.withToken(address(0)) // Native ETH
44+
.build()
45+
);
46+
vm.stopPrank();
47+
}
48+
49+
function testPayClaimInsufficientMsgValueFails() public {
50+
uint256 claimAmount = 10 ether;
51+
uint256 claimId = _createNativeEthClaim(claimAmount);
52+
53+
uint256 paymentAmount = 10 ether;
54+
uint256 insufficientMsgValue = 5 ether; // Less than payment amount
55+
56+
// BullaClaimV2 now correctly validates msg.value == paymentAmount
57+
vm.prank(debtor);
58+
vm.expectRevert(IBullaClaimV2.IncorrectMsgValue.selector);
59+
bullaClaim.payClaim{value: insufficientMsgValue}(claimId, paymentAmount);
60+
}
61+
62+
function testPayClaimInsufficientContractBalanceFails() public {
63+
uint256 claimAmount = 20 ether;
64+
uint256 claimId = _createNativeEthClaim(claimAmount);
65+
66+
uint256 paymentAmount = 10 ether;
67+
uint256 insufficientMsgValue = 3 ether; // Less than payment amount
68+
69+
// With the new validation, msg.value validation happens first
70+
// So insufficient msg.value will revert with IncorrectMsgValue, not ETH_TRANSFER_FAILED
71+
vm.prank(debtor);
72+
vm.expectRevert(IBullaClaimV2.IncorrectMsgValue.selector);
73+
bullaClaim.payClaim{value: insufficientMsgValue}(claimId, paymentAmount);
74+
75+
// Verify claim state remains unchanged
76+
Claim memory claim = bullaClaim.getClaim(claimId);
77+
assertEq(uint256(claim.status), uint256(Status.Pending));
78+
assertEq(claim.paidAmount, 0);
79+
}
80+
81+
function testFuzzPayClaimInsufficientMsgValueFails(uint256 claimAmount, uint256 paymentAmount, uint256 msgValue)
82+
public
83+
{
84+
// Bound inputs to reasonable ranges
85+
claimAmount = bound(claimAmount, 1 ether, 100 ether);
86+
paymentAmount = bound(paymentAmount, 1 ether, claimAmount);
87+
msgValue = bound(msgValue, 0, paymentAmount - 1); // Always less than payment amount
88+
89+
uint256 claimId = _createNativeEthClaim(claimAmount);
90+
91+
// Ensure contract has enough balance to demonstrate the vulnerability
92+
vm.deal(address(bullaClaim), paymentAmount + 100 ether);
93+
94+
vm.prank(debtor);
95+
// Should correctly revert with validation
96+
vm.expectRevert(IBullaClaimV2.IncorrectMsgValue.selector);
97+
bullaClaim.payClaim{value: msgValue}(claimId, paymentAmount);
98+
}
99+
}

‎test/foundry/BullaInvoice/PayInvoiceInsufficientEth.t.sol‎

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
// SPDX-License-Identifier: GPL-2.0-or-later
2+
pragma solidity ^0.8.30;
3+
4+
import "forge-std/console.sol";
5+
import "forge-std/Test.sol";
6+
import "forge-std/Vm.sol";
7+
import "contracts/types/Types.sol";
8+
import {EIP712Helper} from "test/foundry/BullaClaim/EIP712/Utils.sol";
9+
import {BullaClaimV2} from "contracts/BullaClaimV2.sol";
10+
import {BullaInvoice, CreateInvoiceParams, Invoice, IncorrectMsgValue} from "contracts/BullaInvoice.sol";
11+
import {DeployContracts} from "script/DeployContracts.s.sol";
12+
import {CreateInvoiceParamsBuilder} from "test/foundry/BullaInvoice/CreateInvoiceParamsBuilder.sol";
13+
import {InterestConfig} from "contracts/libraries/CompoundInterestLib.sol";
14+
15+
/**
16+
* @title TestPayInvoiceInsufficientEth
17+
* @notice Tests demonstrating BullaInvoice's CORRECT ETH payment validation
18+
* @dev These tests show that BullaInvoice properly validates msg.value == paymentAmount
19+
* This is the secure behavior that BullaClaimV2 should also implement
20+
*/
21+
contract TestPayInvoiceInsufficientEth is Test {
22+
BullaClaimV2 public bullaClaim;
23+
EIP712Helper public sigHelper;
24+
BullaInvoice public bullaInvoice;
25+
26+
uint256 creditorPK = uint256(0x01);
27+
uint256 debtorPK = uint256(0x02);
28+
uint256 adminPK = uint256(0x03);
29+
address creditor = vm.addr(creditorPK);
30+
address debtor = vm.addr(debtorPK);
31+
address admin = vm.addr(adminPK);
32+
33+
function setUp() public {
34+
vm.label(creditor, "CREDITOR");
35+
vm.label(debtor, "DEBTOR");
36+
vm.label(admin, "ADMIN");
37+
38+
DeployContracts.DeploymentResult memory deploymentResult =
39+
(new DeployContracts()).deployForTest(address(this), LockState.Unlocked, 0 ether, 0, 0, address(this));
40+
bullaClaim = BullaClaimV2(deploymentResult.bullaClaim);
41+
sigHelper = new EIP712Helper(address(bullaClaim));
42+
43+
// Create BullaInvoice with 10% protocol fee
44+
bullaInvoice = new BullaInvoice(address(bullaClaim), admin, 1000);
45+
46+
// Setup balances
47+
vm.deal(debtor, 100 ether);
48+
vm.deal(creditor, 100 ether);
49+
vm.deal(admin, 100 ether);
50+
51+
// Send ETH to the invoice contract so it has enough balance to potentially handle payments
52+
vm.deal(address(bullaInvoice), 1000 ether);
53+
}
54+
55+
function _createNativeEthInvoice(uint256 claimAmount) private returns (uint256 invoiceId) {
56+
// Setup permissions for creditor to create invoices
57+
bullaClaim.approvalRegistry().permitCreateClaim({
58+
user: creditor,
59+
controller: address(bullaInvoice),
60+
approvalType: CreateClaimApprovalType.Approved,
61+
approvalCount: 1,
62+
isBindingAllowed: true,
63+
signature: sigHelper.signCreateClaimPermit({
64+
pk: creditorPK,
65+
user: creditor,
66+
controller: address(bullaInvoice),
67+
approvalType: CreateClaimApprovalType.Approved,
68+
approvalCount: 1,
69+
isBindingAllowed: true
70+
})
71+
});
72+
73+
// Create invoice with native ETH (address(0))
74+
CreateInvoiceParams memory params = new CreateInvoiceParamsBuilder().withDebtor(debtor).withCreditor(creditor)
75+
.withToken(address(0)).withClaimAmount(claimAmount).withLateFeeConfig(
76+
InterestConfig({interestRateBps: 0, numberOfPeriodsPerYear: 0})
77+
) // Native ETH
78+
// No interest for simplicity
79+
.build();
80+
81+
uint256 fee = bullaClaim.CORE_PROTOCOL_FEE();
82+
83+
vm.prank(creditor);
84+
invoiceId = bullaInvoice.createInvoice{value: fee}(params);
85+
}
86+
87+
function testPayInvoiceInsufficientMsgValue() public {
88+
uint256 claimAmount = 10 ether;
89+
uint256 invoiceId = _createNativeEthInvoice(claimAmount);
90+
91+
uint256 paymentAmount = 10 ether;
92+
uint256 insufficientMsgValue = 5 ether; // Less than payment amount
93+
94+
// Verify the contract has enough ETH to potentially handle the transfer
95+
assertGe(address(bullaInvoice).balance, paymentAmount, "Invoice contract should have enough ETH for payment");
96+
97+
// Attempt to pay with insufficient msg.value - should revert with IncorrectMsgValue
98+
vm.prank(debtor);
99+
vm.expectRevert(IncorrectMsgValue.selector);
100+
bullaInvoice.payInvoice{value: insufficientMsgValue}(invoiceId, paymentAmount);
101+
102+
// Verify invoice state remains unchanged
103+
Invoice memory invoice = bullaInvoice.getInvoice(invoiceId);
104+
assertEq(uint256(invoice.status), uint256(Status.Pending));
105+
assertEq(invoice.paidAmount, 0);
106+
}
107+
108+
function testPayInvoicePartialInsufficientMsgValue() public {
109+
uint256 claimAmount = 20 ether;
110+
uint256 invoiceId = _createNativeEthInvoice(claimAmount);
111+
112+
uint256 paymentAmount = 8 ether;
113+
uint256 insufficientMsgValue = 3 ether; // Less than payment amount
114+
115+
// Verify the contract has enough ETH to potentially handle the transfer
116+
assertGe(address(bullaInvoice).balance, paymentAmount, "Invoice contract should have enough ETH for payment");
117+
118+
vm.prank(debtor);
119+
vm.expectRevert(IncorrectMsgValue.selector);
120+
bullaInvoice.payInvoice{value: insufficientMsgValue}(invoiceId, paymentAmount);
121+
122+
// Verify invoice state remains unchanged
123+
Invoice memory invoice = bullaInvoice.getInvoice(invoiceId);
124+
assertEq(uint256(invoice.status), uint256(Status.Pending));
125+
assertEq(invoice.paidAmount, 0);
126+
}
127+
128+
function testPayInvoiceWithInterestInsufficientMsgValue() public {
129+
uint256 claimAmount = 10 ether;
130+
131+
// Create invoice with interest
132+
bullaClaim.approvalRegistry().permitCreateClaim({
133+
user: creditor,
134+
controller: address(bullaInvoice),
135+
approvalType: CreateClaimApprovalType.Approved,
136+
approvalCount: 1,
137+
isBindingAllowed: true,
138+
signature: sigHelper.signCreateClaimPermit({
139+
pk: creditorPK,
140+
user: creditor,
141+
controller: address(bullaInvoice),
142+
approvalType: CreateClaimApprovalType.Approved,
143+
approvalCount: 1,
144+
isBindingAllowed: true
145+
})
146+
});
147+
148+
CreateInvoiceParams memory params = new CreateInvoiceParamsBuilder().withDebtor(debtor).withCreditor(creditor)
149+
.withToken(address(0)).withClaimAmount(claimAmount).withLateFeeConfig(
150+
InterestConfig({interestRateBps: 1000, numberOfPeriodsPerYear: 12})
151+
) // Native ETH
152+
// 10% annual interest
153+
.build();
154+
155+
uint256 fee = bullaClaim.CORE_PROTOCOL_FEE();
156+
157+
vm.prank(creditor);
158+
uint256 invoiceId = bullaInvoice.createInvoice{value: fee}(params);
159+
160+
// Fast forward to accrue interest
161+
vm.warp(block.timestamp + 90 days);
162+
163+
Invoice memory invoice = bullaInvoice.getInvoice(invoiceId);
164+
uint256 accruedInterest = invoice.interestComputationState.accruedInterest;
165+
uint256 paymentAmount = accruedInterest + 5 ether; // Interest + partial principal
166+
uint256 insufficientMsgValue = paymentAmount - 1 ether; // Less than required
167+
168+
// Should revert with IncorrectMsgValue when paying interest + principal with insufficient msg.value
169+
vm.prank(debtor);
170+
vm.expectRevert(IncorrectMsgValue.selector);
171+
bullaInvoice.payInvoice{value: insufficientMsgValue}(invoiceId, paymentAmount);
172+
}
173+
174+
function testFuzzPayInvoiceInsufficientMsgValue(uint256 claimAmount, uint256 paymentAmount, uint256 msgValue)
175+
public
176+
{
177+
// Bound inputs to reasonable ranges
178+
claimAmount = bound(claimAmount, 1 ether, 100 ether);
179+
paymentAmount = bound(paymentAmount, 1 ether, claimAmount);
180+
msgValue = bound(msgValue, 0, paymentAmount - 1); // Always less than payment amount
181+
182+
uint256 invoiceId = _createNativeEthInvoice(claimAmount);
183+
184+
// Ensure contract has enough ETH
185+
vm.deal(address(bullaInvoice), claimAmount + 100 ether);
186+
187+
vm.prank(debtor);
188+
vm.expectRevert(IncorrectMsgValue.selector); // Should always revert when msg.value < paymentAmount
189+
bullaInvoice.payInvoice{value: msgValue}(invoiceId, paymentAmount);
190+
}
191+
}

0 commit comments

Comments
 (0)