diff --git a/foundry.toml b/foundry.toml index d767d5d..9efaaa0 100644 --- a/foundry.toml +++ b/foundry.toml @@ -4,7 +4,7 @@ out = "out" libs = ["lib"] optimizer = true -optimizer_runs = 200 +optimizer_runs = 1 # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options diff --git a/src/sales/SettlementSale.sol b/src/sales/SettlementSale.sol index 975fe86..fedacff 100644 --- a/src/sales/SettlementSale.sol +++ b/src/sales/SettlementSale.sol @@ -94,6 +94,8 @@ import {TokenAmount, WalletTokenAmount} from "sales/interfaces/types.sol"; /// The `entityID` refers to an entity in the Sonar system, which can be either a legal entity or an individual. /// A wallet is an address used to commit funds to the sale. /// An entity can have multiple wallets, but each wallet is associated with exactly one entity. +/// All wallets under the same entity are mutually trusted: any wallet can cancel or reduce commitments +/// for any other wallet in the entity. Funds are always returned to the committing wallet, not the caller. /// /// With the exception of the emergency recovery mechanism, tokens can only be: /// - transferred to the contract as part of a bid @@ -152,6 +154,10 @@ contract SettlementSale is /// @notice The role allowed to refund entities. bytes32 public constant REFUNDER_ROLE = keccak256("REFUNDER_ROLE"); + /// @notice The role allowed to reduce commitments on behalf of entities. + /// @dev This is not granted by default. It should be granted manually by the DEFAULT_ADMIN_ROLE when needed. + bytes32 public constant COMMITMENT_REDUCER_ROLE = keccak256("COMMITMENT_REDUCER_ROLE"); + // Initialization errors error InvalidPaymentTokenDecimals(address token, uint256 got, uint256 want); error DuplicatePaymentToken(address token); @@ -188,8 +194,8 @@ contract SettlementSale is // Cancellation errors error ReduceCommitmentDisabled(); - error ReductionExceedsCommitment( - bytes16 entityID, address wallet, address token, uint256 amount, uint256 committed + error ReductionExceedsReducibleAmount( + bytes16 entityID, address wallet, address token, uint256 amount, uint256 committed, uint256 allocated ); // Refund errors @@ -508,10 +514,16 @@ contract SettlementSale is } for (uint256 i = 0; i < init.extraManagers.length; i++) { + if (init.extraManagers[i] == address(0)) { + revert ZeroAddress(); + } _grantRole(SALE_MANAGER_ROLE, init.extraManagers[i]); } for (uint256 i = 0; i < init.extraPausers.length; i++) { + if (init.extraPausers[i] == address(0)) { + revert ZeroAddress(); + } _grantRole(PAUSER_ROLE, init.extraPausers[i]); } @@ -645,6 +657,7 @@ contract SettlementSale is /// @notice Processes a bid during the `Commitment` stage, validating the purchase permit, any constraints specified on the permit, and updating the bid. /// @dev The minimum and maximum total bid amount and the minimum and maximum price are specified on the purchase permit (`minAmount`, `maxAmount`, `minPrice`, and `maxPrice`, respectively). + /// `minAmount` is enforced only at bid submission. It is not stored onchain and does not constrain subsequent reductions during the `Cancellation` stage. function _processBid( IERC20 token, Bid calldata newBid, @@ -687,8 +700,9 @@ contract SettlementSale is } EntityState storage state = _entityStateByID[purchasePermit.saleSpecificEntityID]; - // additional safety check: to avoid any bookkeeping issues, we disallow new bids for entities that have already been refunded. - // this can theoretically happen if the commitment stage was reopened after already refunding some entities. + // since already refunded entities cannot be refunded again, we disallow new bids for them to avoid any bookkeeping issues. + // while this cannot happen in the normal flow of the sale, it is theoretically possible if the commitment stage is reopened + // through unsafeSetStage after some entities have already been refunded. if (state.refunded) { revert AlreadyRefunded(purchasePermit.saleSpecificEntityID); } @@ -754,8 +768,10 @@ contract SettlementSale is _setStage(Stage.Cancellation); } - /// @notice Fully cancels an entity's bid during the `Cancellation` stage, refunding all committed amounts. - /// @dev Can be called by any wallet associated with the entity. Always available regardless of `reduceCommitmentEnabled`. + /// @notice Fully cancels an entity's bid during the `Cancellation` stage, refunding all unaccepted committed amounts. + /// @dev Can be called by any wallet associated with the entity. + /// Always available regardless of `reduceCommitmentEnabled`. + /// For intentional partial reductions, use `reduceCommitment()` instead. function cancelBid() external onlyStage(Stage.Cancellation) onlyUnpaused { bytes16 entityID = _entityIDByAddress[msg.sender]; if (entityID == bytes16(0)) { @@ -772,9 +788,11 @@ contract SettlementSale is uint256 numTokens = _paymentTokens.length; for (uint256 i = 0; i < wallets.length; i++) { WalletState storage walletState = state.walletStates[wallets[i]]; + for (uint256 j = 0; j < numTokens; j++) { IERC20 token = _paymentTokens[j]; - uint256 amount = walletState.committedAmountByToken[token]; + uint256 amount = walletState.committedAmountByToken[token] - walletState.acceptedAmountByToken[token]; + if (amount > 0) { _reduceCommitment(entityID, wallets[i], token, amount); } @@ -783,7 +801,12 @@ contract SettlementSale is } /// @notice Partially reduces specific wallet/token commitments during the `Cancellation` stage. + /// @dev Only processes the caller-supplied tuples. Any wallet/token pairs not included in `reductions` are left + /// unchanged, and the entity's remaining committed balance will be carried into settlement. + /// For full cancellation, use `cancelBid()` which reads all pairs from contract storage. /// @dev Requires `reduceCommitmentEnabled`. The caller must be a wallet associated with the entity. + /// @dev No minimum floor is enforced on the resulting commitment. The `minAmount` constraint from the purchase permit + /// applies only at bid submission and entities may reduce below that threshold here. /// @param reductions Array of (wallet, token, amount) tuples specifying what to reduce. function reduceCommitment(WalletTokenAmount[] calldata reductions) external @@ -813,6 +836,9 @@ contract SettlementSale is /// @notice Reduces a wallet's commitment for a given token by `amount` and transfers the funds back. function _reduceCommitment(bytes16 entityID, address wallet, IERC20 token, uint256 amount) internal { EntityState storage state = _entityStateByID[entityID]; + if (state.refunded) { + revert AlreadyRefunded(entityID); + } if (!state.wallets.contains(wallet)) { revert WalletNotAssociatedWithEntity(wallet, entityID); @@ -827,10 +853,10 @@ contract SettlementSale is } WalletState storage walletState = state.walletStates[wallet]; - if (walletState.committedAmountByToken[token] < amount) { - revert ReductionExceedsCommitment( - entityID, wallet, address(token), amount, walletState.committedAmountByToken[token] - ); + uint256 committed = walletState.committedAmountByToken[token]; + uint256 allocated = walletState.acceptedAmountByToken[token]; + if (amount > committed - allocated) { + revert ReductionExceedsReducibleAmount(entityID, wallet, address(token), amount, committed, allocated); } walletState.committedAmountByToken[token] -= amount; @@ -1328,11 +1354,28 @@ contract SettlementSale is /// @notice Recovers any ERC20 tokens that are sent to the contract. /// @dev This can be used to recover any tokens that are sent to the contract by mistake. + /// Use `forceReduceCommitment` which updates accounting state instead if possible. function recoverTokens(IERC20 token, uint256 amount, address to) external onlyRole(TOKEN_RECOVERER_ROLE) { emit TokensRecovered(address(token), amount, to); token.safeTransfer(to, amount); } + /// @notice Sale operator initiated reduction of wallet commitments, bypassing stage and pause restrictions. + /// @dev This should be preferred over `recoverTokens()` for committed payment tokens, since it + /// correctly updates all accounting state (committed/cancelled amounts, bid totals, global counters). + function forceReduceCommitment(WalletTokenAmount[] calldata reductions) external onlyRole(COMMITMENT_REDUCER_ROLE) { + for (uint256 i = 0; i < reductions.length; i++) { + WalletTokenAmount calldata c = reductions[i]; + + bytes16 entityID = _entityIDByAddress[c.wallet]; + if (entityID == bytes16(0)) { + revert WalletNotInitialized(c.wallet); + } + + _reduceCommitment(entityID, c.wallet, IERC20(c.token), c.amount); + } + } + /// @notice Checks if the contract supports an interface. function supportsInterface(bytes4 interfaceId) public diff --git a/test/Cancellation.t.sol b/test/Cancellation.t.sol index ca05497..3f56ee3 100644 --- a/test/Cancellation.t.sol +++ b/test/Cancellation.t.sol @@ -4,6 +4,16 @@ pragma solidity ^0.8.23; import "./SettlementSaleBaseTest.t.sol"; contract SettlementSaleCancellationTest is SettlementSaleBaseTest { + address internal immutable reducer = makeAddr("reducer"); + + function setUp() public virtual override { + super.setUp(); + + vm.startPrank(admin); + sale.grantRole(sale.COMMITMENT_REDUCER_ROLE(), reducer); + vm.stopPrank(); + } + struct State { uint256 bidAmount; bool refunded; @@ -406,7 +416,13 @@ contract SettlementSaleCancellationTest is SettlementSaleBaseTest { vm.expectRevert( abi.encodeWithSelector( - SettlementSale.ReductionExceedsCommitment.selector, aliceID, alice, address(usdc), 6000e6, 5000e6 + SettlementSale.ReductionExceedsReducibleAmount.selector, + aliceID, + alice, + address(usdc), + 6000e6, + 5000e6, + 0 ) ); vm.prank(alice); @@ -579,7 +595,13 @@ contract SettlementSaleCancellationTest is SettlementSaleBaseTest { vm.expectRevert( abi.encodeWithSelector( - SettlementSale.ReductionExceedsCommitment.selector, aliceID, alice, address(usdc), 3000e6, 2000e6 + SettlementSale.ReductionExceedsReducibleAmount.selector, + aliceID, + alice, + address(usdc), + 3000e6, + 2000e6, + 0 ) ); vm.prank(alice); @@ -757,4 +779,234 @@ contract SettlementSaleCancellationTest is SettlementSaleBaseTest { assertEq(usdc.balanceOf(alice), 2500e6, "alice balance after two cancels"); assertEq(sale.totalCancelledAmount(), 2500e6, "total cancelled"); } + + // --- Finding 14: _reduceCommitment caps at committed - accepted --- + + function testCancelBid_AfterUnsafeSetStageBack_CapsAtUnacceptedAmount() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + // Set allocations during settlement + openSettlement(); + doSetAllocation(alice, usdc, 3000e6); + + // Admin moves sale back to Cancellation + vm.prank(admin); + sale.unsafeSetStage(SettlementSale.Stage.Cancellation); + + // cancelBid should only refund the unaccepted portion (5000 - 3000 = 2000) + vm.prank(alice); + sale.cancelBid(); + + assertEq(usdc.balanceOf(alice), 2000e6, "alice should only get unaccepted amount back"); + assertEq(usdc.balanceOf(address(sale)), 3000e6, "sale should retain accepted amount"); + } + + function testReduceCommitment_AfterUnsafeSetStageBack_CapsAtUnacceptedAmount() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + openSettlement(); + doSetAllocation(alice, usdc, 3000e6); + + vm.prank(admin); + sale.unsafeSetStage(SettlementSale.Stage.Cancellation); + enableReduceCommitment(); + + // Try to reduce full committed amount: should revert because max reducible is 2000 + WalletTokenAmount[] memory reductions = new WalletTokenAmount[](1); + reductions[0] = WalletTokenAmount({wallet: alice, token: address(usdc), amount: 5000e6}); + + vm.expectRevert( + abi.encodeWithSelector( + SettlementSale.ReductionExceedsReducibleAmount.selector, + aliceID, + alice, + address(usdc), + 5000e6, + 5000e6, + 3000e6 + ) + ); + vm.prank(alice); + sale.reduceCommitment(reductions); + + // Reducing the unaccepted portion should succeed + reductions[0] = WalletTokenAmount({wallet: alice, token: address(usdc), amount: 2000e6}); + vm.prank(alice); + sale.reduceCommitment(reductions); + + assertEq(usdc.balanceOf(alice), 2000e6, "alice should get unaccepted amount back"); + assertEq(sale.entityStateByID(aliceID).currentBid.amount, 3000e6, "bid should reflect accepted amount"); + } + + // --- Finding 2: forceReduceCommitment --- + + function toForceReductions( + address wallet, + address token, + uint256 amount + ) internal pure returns (WalletTokenAmount[] memory) { + WalletTokenAmount[] memory reductions = new WalletTokenAmount[](1); + reductions[0] = WalletTokenAmount({wallet: wallet, token: token, amount: amount}); + return reductions; + } + + function testForceReduceCommitment_FullAmount_RefundsAndMarksRefunded() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 5000e6)); + + assertEq(usdc.balanceOf(alice), 5000e6, "alice should receive tokens back"); + assertTrue(sale.entityStateByID(aliceID).refunded, "entity should be marked refunded"); + assertEq(sale.entityStateByID(aliceID).currentBid.amount, 0, "bid amount should be zero"); + assertEq(sale.totalCancelledAmount(), 5000e6, "total cancelled should be updated"); + } + + function testForceReduceCommitment_PartialAmount_UpdatesAccounting() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 2000e6)); + + assertEq(usdc.balanceOf(alice), 2000e6, "alice should get partial refund"); + assertEq(sale.entityStateByID(aliceID).currentBid.amount, 3000e6, "bid should be reduced"); + assertFalse(sale.entityStateByID(aliceID).refunded, "should not be fully refunded"); + + SettlementSale.WalletStateView memory walletState = sale.walletStateByAddress(alice); + assertEq(walletState.committedAmountByToken[0].amount, 3000e6, "committed should be reduced"); + assertEq(walletState.cancelledAmountByToken[0].amount, 2000e6, "cancelled should be tracked"); + } + + function testForceReduceCommitment_NoStageRestriction() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + // Should work during Commitment stage + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 1000e6)); + assertEq(usdc.balanceOf(alice), 1000e6, "should work in Commitment stage"); + + // Should work during Settlement stage + openSettlement(); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 1000e6)); + assertEq(usdc.balanceOf(alice), 2000e6, "should work in Settlement stage"); + } + + function testForceReduceCommitment_CapsAtUnacceptedAmount() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + openSettlement(); + doSetAllocation(alice, usdc, 3000e6); + + vm.expectRevert( + abi.encodeWithSelector( + SettlementSale.ReductionExceedsReducibleAmount.selector, + aliceID, + alice, + address(usdc), + 5000e6, + 5000e6, + 3000e6 + ) + ); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 5000e6)); + + // Should succeed with unaccepted amount + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 2000e6)); + assertEq(usdc.balanceOf(alice), 2000e6, "should refund unaccepted amount"); + } + + function testForceReduceCommitment_UnauthorizedUser_Reverts() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + vm.expectRevert(missingRoleError(alice, sale.COMMITMENT_REDUCER_ROLE())); + vm.prank(alice); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 5000e6)); + } + + function testForceReduceCommitment_UninitializedWallet_Reverts() public { + vm.expectRevert(abi.encodeWithSelector(SettlementSale.WalletNotInitialized.selector, alice)); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 1000e6)); + } + + function testForceReduceCommitment_ZeroAmount_Reverts() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + vm.expectRevert(abi.encodeWithSelector(SettlementSale.ZeroAmount.selector)); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 0)); + } + + function testForceReduceCommitment_AfterClaimRefund_Reverts() public { + // Reproduces double-dip: user claims refund, then forceReduceCommitment extracts more funds. + // + // Setup: alice commits 1000, gets settled for 600, claims 400 refund. + // Bug: forceReduceCommitment sees committed(1000) - allocated(600) = 400 reducible, + // allowing a second 400 transfer despite the refund already being claimed. + openCommitment(); + doBid({user: alice, amount: 1000e6, price: 10, token: usdc}); + + openSettlement(); + doSetAllocation(alice, usdc, 600e6); + finalizeSettlement(); + + // Alice claims her 400 refund + vm.prank(alice); + sale.claimRefund(); + assertEq(usdc.balanceOf(alice), 400e6); + assertTrue(sale.entityStateByID(aliceID).refunded); + + // forceReduceCommitment should revert since the entity is already refunded + vm.expectRevert(abi.encodeWithSelector(SettlementSale.AlreadyRefunded.selector, aliceID)); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 400e6)); + + // Balance unchanged -- no double-dip + assertEq(usdc.balanceOf(alice), 400e6); + } + + function testForceReduceCommitment_AfterProcessRefunds_Reverts() public { + openCommitment(); + doBid({user: alice, amount: 1000e6, price: 10, token: usdc}); + + openSettlement(); + doSetAllocation(alice, usdc, 600e6); + finalizeSettlement(); + + // Refunder processes alice's refund + bytes16[] memory entityIDs = new bytes16[](1); + entityIDs[0] = aliceID; + vm.prank(refunder); + sale.processRefunds(entityIDs, false); + assertEq(usdc.balanceOf(alice), 400e6); + + // forceReduceCommitment should also revert + vm.expectRevert(abi.encodeWithSelector(SettlementSale.AlreadyRefunded.selector, aliceID)); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, address(usdc), 400e6)); + + assertEq(usdc.balanceOf(alice), 400e6); + } + + function testForceReduceCommitment_InvalidToken_Reverts() public { + openCommitment(); + doBid({user: alice, amount: 5000e6, price: 10, token: usdc}); + + address fakeToken = makeAddr("fakeToken"); + + vm.expectRevert(abi.encodeWithSelector(SettlementSale.InvalidPaymentToken.selector, fakeToken)); + vm.prank(reducer); + sale.forceReduceCommitment(toForceReductions(alice, fakeToken, 1000e6)); + } } diff --git a/test/General.t.sol b/test/General.t.sol index 0db0d8f..1e7ef7f 100644 --- a/test/General.t.sol +++ b/test/General.t.sol @@ -83,6 +83,7 @@ contract SettlementSaleConstructorTest is BaseTest { assertEq(sale.getRoleMember(sale.PURCHASE_PERMIT_SIGNER_ROLE(), 0), permitSigner.addr); assertEq(sale.getRoleMemberCount(sale.TOKEN_RECOVERER_ROLE()), 0); + assertEq(sale.getRoleMemberCount(sale.COMMITMENT_REDUCER_ROLE()), 0); } function testConstructor_NoExtraRoles_DeploysSuccessfully() public { @@ -307,6 +308,60 @@ contract SettlementSaleConstructorTest is BaseTest { testSale.initialize(init); } + function testInitialize_ZeroExtraManager_Reverts() public { + address[] memory extraManagers = new address[](2); + extraManagers[0] = manager; + extraManagers[1] = address(0); + + SettlementSale.Init memory init = SettlementSale.Init({ + saleUUID: TEST_SALE_UUID, + admin: admin, + extraManagers: extraManagers, + purchasePermitSigner: permitSigner.addr, + proceedsReceiver: receiver, + extraPausers: defaultPausers, + extraSettler: settler, + extraRefunder: refunder, + claimRefundEnabled: true, + maxWalletsPerEntity: 50, + paymentTokens: _defaultPaymentTokens(), + expectedPaymentTokenDecimals: 6, + reduceCommitmentEnabled: false, + skipPreOpen: false + }); + + TestableSettlementSale testSale = newUninitializedTestableSettlementSale(); + vm.expectRevert(abi.encodeWithSelector(SettlementSale.ZeroAddress.selector)); + testSale.initialize(init); + } + + function testInitialize_ZeroExtraPauser_Reverts() public { + address[] memory extraPausers = new address[](2); + extraPausers[0] = pauser; + extraPausers[1] = address(0); + + SettlementSale.Init memory init = SettlementSale.Init({ + saleUUID: TEST_SALE_UUID, + admin: admin, + extraManagers: defaultExtraManagers, + purchasePermitSigner: permitSigner.addr, + proceedsReceiver: receiver, + extraPausers: extraPausers, + extraSettler: settler, + extraRefunder: refunder, + claimRefundEnabled: true, + maxWalletsPerEntity: 50, + paymentTokens: _defaultPaymentTokens(), + expectedPaymentTokenDecimals: 6, + reduceCommitmentEnabled: false, + skipPreOpen: false + }); + + TestableSettlementSale testSale = newUninitializedTestableSettlementSale(); + vm.expectRevert(abi.encodeWithSelector(SettlementSale.ZeroAddress.selector)); + testSale.initialize(init); + } + function testInitialize_ZeroMaxWalletsPerEntity_Reverts() public { SettlementSale.Init memory init = SettlementSale.Init({ saleUUID: TEST_SALE_UUID,