[Fil 1399] [Client sc] remove longest deal term

src/Client.sol

@@ -164,7 +164,6 @@ contract Client is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Ree
         uint256 dealId;
         uint256 railId;
         uint256 sizeOfAllocations;
-        CommonTypes.ChainEpoch maxAllocationEndTime;
         CommonTypes.FilActorId[] allocationIds;
     }
 
@@ -259,15 +258,13 @@ contract Client is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Ree
         }
 
         if (msg.sender != deal.client) revert InvalidClient();
-        (ProviderAllocation[] memory allocations, ProviderClaim[] memory claimExtensions, int64 maxAllocationEndTime) =
+        (ProviderAllocation[] memory allocations, ProviderClaim[] memory claimExtensions) =
             _deserializeVerifregOperatorData(params.operator_data);
 
-        if (maxAllocationEndTime > CommonTypes.ChainEpoch.unwrap(deal.maxAllocationEndTime)) {
-            deal.maxAllocationEndTime = CommonTypes.ChainEpoch.wrap(maxAllocationEndTime);
-        }
         uint256 sizeOfAllocations = _verifyAndRegisterAllocations(dealId, allocations);
         uint256 sizeOfClaims = _verifyAndRegisterClaimExtensions(dealId, claimExtensions);
         uint256 allocationsAndClaimsSize = sizeOfAllocations + sizeOfClaims;
+
         $._metaAllocatorContract
             .addVerifiedClient(FilAddresses.fromEthAddress(address(this)).data, allocationsAndClaimsSize);
 
@@ -333,16 +330,11 @@ contract Client is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Ree
      * @param cborData The cbor encoded operator data.
      * @return allocations Array of provider allocations.
      * @return claimExtensions Array of provider claims.
-     * @return maxAllocationEndTime Allocation with the longest term.
      */
     function _deserializeVerifregOperatorData(bytes memory cborData)
         internal
         pure
-        returns (
-            ProviderAllocation[] memory allocations,
-            ProviderClaim[] memory claimExtensions,
-            int64 maxAllocationEndTime
-        )
+        returns (ProviderAllocation[] memory allocations, ProviderClaim[] memory claimExtensions)
     {
         uint256 resultLength;
         uint64 provider;
@@ -353,8 +345,6 @@ contract Client is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Ree
 
         {
             uint64 size;
-            int64 termMax;
-            int64 expiration;
             (resultLength, byteIdx) = CBORDecoder.readFixedArray(cborData, byteIdx);
             allocations = new ProviderAllocation[](resultLength);
             for (uint256 i = 0; i < resultLength; i++) {
@@ -376,15 +366,9 @@ contract Client is Initializable, AccessControlUpgradeable, UUPSUpgradeable, Ree
                     allocations[i].size = size;
                 }
                 (, byteIdx) = CBORDecoder.readInt64(cborData, byteIdx); // termMin
+                (, byteIdx) = CBORDecoder.readInt64(cborData, byteIdx); // termMax
+                (, byteIdx) = CBORDecoder.readInt64(cborData, byteIdx); // expiration
                 // slither-disable-end unused-return
-                {
-                    (termMax, byteIdx) = CBORDecoder.readInt64(cborData, byteIdx);
-                    (expiration, byteIdx) = CBORDecoder.readInt64(cborData, byteIdx);
-
-                    if (termMax + expiration > maxAllocationEndTime) {
-                        maxAllocationEndTime = termMax + expiration;
-                    }
-                }
             }
         }
         {

test/Client.t.sol

@@ -24,7 +24,7 @@ import {ActorIdExitCodeErrorFailingMock} from "./contracts/ActorIdExitCodeErrorF
 import {FailingMockAddVerifiedClient} from "./contracts/FailingMockAddVerifiedClient.sol";
 import {AllocationResponseCbor} from "../src/lib/AllocationResponseCbor.sol";
 import {ClientContractMock} from "./contracts/ClientContractMock.sol";
-import {ReentrantValidatorMock} from "./contracts/ReentrantValidatorMock.sol";
+import {ReentrantMetaAllocatorMock} from "./contracts/ReentrantMetaAllocatorMock.sol";
 import {SLITypes} from "../src/types/SLITypes.sol";
 import {PoRepTypes} from "../src/types/PoRepTypes.sol";
 import {MetaAllocatorMock} from "./contracts/MetaAllocatorMock.sol";
@@ -132,7 +132,6 @@ contract ClientTest is Test {
     }
 
     function setupProxy(address impl) public returns (address) {
-        // solhint-disable-next-line gas-small-strings
         bytes memory initData = abi.encodeCall(
             Client.initialize, (address(this), terminationOracle, address(poRepMarketMock), address(metaAllocatorMock))
         );
@@ -406,6 +405,8 @@ contract ClientTest is Test {
         // solhint-disable-next-line reentrancy
         transferParams.operator_data =
             hex"828286192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A0050334019013186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221950001A0007E9001A009C7E801901318183192710011A005034AC";
+        vm.expectEmit(true, true, true, true);
+        emit Client.DatacapSpent(clientAddress, 24576);
         clientMock.transfer(transferParams, dealId, false);
 
         Client.Deal memory deal = clientMock.getDeal(dealId);
@@ -416,89 +417,36 @@ contract ClientTest is Test {
         assertEq(deal.client, clientAddress);
     }
 
-    // solhint-disable reentrancy
-    function testShouldUpdateMaxAllocationEndTimeWhenNewDealIsLongerThanCurrent() public {
-        ClientContractMock clientMock = ClientContractMock(setupProxy(address(new ClientContractMock())));
-        metaAllocatorMock.setAllowance(address(clientMock), uint256(1000000));
-        int64 expectedMaxAllocationEndTimeBefore = 5256305;
-        int64 expectedMaxAllocationEndTimeAfter = 10256305;
-
-        // termMax + expiration -> 5256305
-        transferParams.operator_data =
-            hex"828186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A005033401901318183192710011A005034AC";
-        vm.prank(clientAddress);
-        clientMock.transfer(transferParams, dealId, false);
-
-        Client.Deal memory deal = clientMock.getDeal(dealId);
-        assertTrue(CommonTypes.ChainEpoch.unwrap(deal.maxAllocationEndTime) == expectedMaxAllocationEndTimeBefore);
-
-        // termMax + expiration -> 10256305
-        transferParams.operator_data =
-            hex"828286192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A0050334019013186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221950001A0007E9001A009C7E801901318183192710011A005034AC";
-        vm.prank(clientAddress);
-        clientMock.transfer(transferParams, dealId, false);
-
-        deal = clientMock.getDeal(dealId);
-        assertTrue(CommonTypes.FilActorId.unwrap(deal.provider) == CommonTypes.FilActorId.unwrap(SP1));
-        assertEq(deal.dealId, dealId);
-        assertEq(deal.validator, address(validatorMock));
-        assertEq(deal.railId, 0);
-        assertEq(deal.client, clientAddress);
-        assertTrue(CommonTypes.ChainEpoch.unwrap(deal.maxAllocationEndTime) == expectedMaxAllocationEndTimeAfter);
-    }
-
-    function testShouldNotUpdateMaxAllocationEndTimeWhenNewDealIsShorterThanCurrent() public {
-        ClientContractMock clientMock = ClientContractMock(setupProxy(address(new ClientContractMock())));
-        metaAllocatorMock.setAllowance(address(clientMock), uint256(1000000));
-        int64 expectedMaxAllocationEndTimeBefore = 5256305;
-        int64 expectedMaxAllocationEndTimeAfter = expectedMaxAllocationEndTimeBefore;
-
-        // termMax + expiration -> 5256305 from operator_data
-        transferParams.operator_data =
-            hex"828186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A005033401901318183192710011A005034AC";
-        vm.prank(clientAddress);
-        clientMock.transfer(transferParams, dealId, false);
-
-        Client.Deal memory deal = clientMock.getDeal(dealId);
-        assertTrue(CommonTypes.ChainEpoch.unwrap(deal.maxAllocationEndTime) == expectedMaxAllocationEndTimeBefore);
-
-        // termMax + expiration -> 2256305 from operator_data
-        transferParams.operator_data =
-            hex"828286192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A0050334019013186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221950001A0007E9001A00226C801901318183192710011A005034AC";
-        vm.prank(clientAddress);
-        clientMock.transfer(transferParams, dealId, false);
-
-        deal = clientMock.getDeal(dealId);
-        assertTrue(CommonTypes.FilActorId.unwrap(deal.provider) == CommonTypes.FilActorId.unwrap(SP1));
-        assertEq(deal.dealId, dealId);
-        assertEq(deal.validator, address(validatorMock));
-        assertEq(deal.railId, 0);
-        assertEq(deal.client, clientAddress);
-        assertTrue(CommonTypes.ChainEpoch.unwrap(deal.maxAllocationEndTime) == expectedMaxAllocationEndTimeAfter);
-    }
+    function testReentryAttemptWillThrowInvalidClientError() public {
+        ReentrantMetaAllocatorMock reentrantMetaAllocatorMock = new ReentrantMetaAllocatorMock();
+        address impl = address(new Client());
+        bytes memory initData = abi.encodeCall(
+            Client.initialize,
+            (address(this), terminationOracle, address(poRepMarketMock), address(reentrantMetaAllocatorMock))
+        );
+        Client clientWithReentrancy = Client(address(new ERC1967Proxy(address(impl), initData)));
 
-    // solhint-enable reentrancy
-    function testShouldNotTransferIfReentrantCall() public {
-        ReentrantValidatorMock reentrantValidatorMock = new ReentrantValidatorMock();
         poRepMarketMock.setDealProposal(
             dealId,
             PoRepTypes.DealProposal({
-                dealId: 150,
+                dealId: dealId,
                 client: clientAddress,
                 provider: SP1,
                 requirements: SLITypes.SLIThresholds({
                     retrievabilityBps: 80, bandwidthMbps: 500, latencyMs: 200, indexingPct: 90
                 }),
                 terms: SLITypes.DealTerms({dealSizeBytes: 1024, pricePerSector: 100, durationDays: 365}),
-                validator: address(reentrantValidatorMock),
                 state: PoRepTypes.DealState.Accepted,
+                validator: address(validatorMock),
                 railId: 0,
                 manifestLocation: expectedManifestLocation
             })
         );
-        reentrantValidatorMock.setAttackParams(address(client), transferParams, dealId);
+        reentrantMetaAllocatorMock.setAttackParams(address(clientWithReentrancy), transferParams, dealId);
         vm.prank(clientAddress);
         client.transfer(transferParams, dealId, true);
+        vm.expectRevert(abi.encodeWithSelector(Client.InvalidClient.selector));
+        clientWithReentrancy.transfer(transferParams, dealId, false);
     }
 
     function testShouldAddClaimExtensionIdsAfterTransfer() public {
@@ -531,6 +479,8 @@ contract ClientTest is Test {
         transferParams.operator_data =
             hex"828286192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221908001A0007E9001A0050334019013186192710D82A5828000181E203922020F2B9A58BBC9D9856E52EAB85155C1BA298F7E8DF458BD20A3AD767E11572CA221950001A0007E9001A009C7E801901318183192710041A005034AC";
         actorIdMock.setDataCapTransferResult(hex"834100410049838201808200808102");
+        vm.expectEmit(true, true, true, true);
+        emit Client.DatacapSpent(clientAddress, 24576);
 
         vm.prank(clientAddress);
         clientMock.transfer(transferParams, dealId, false);

test/contracts/ClientSCMock.sol

@@ -24,10 +24,6 @@ contract ClientSCMock {
         return dataSizeMatches[dealId];
     }
 
-    function setLongestDealTerm(uint256 dealId, int64 longestDealTerm) external {
-        deals[dealId].maxAllocationEndTime = CommonTypes.ChainEpoch.wrap(longestDealTerm);
-    }
-
     function getClientDealInfo(uint256 dealId) external view returns (Client.Deal memory) {
         return deals[dealId];
     }

test/contracts/ReentrantValidatorMock.sol → test/contracts/ReentrantMetaAllocatorMock.sol

@@ -2,12 +2,11 @@
 // solhint-disable use-natspec
 pragma solidity =0.8.30;
 
-import {IValidator} from "../../src/interfaces/Validator.sol";
+import {IMetaAllocator} from "../../src/interfaces/IMetaAllocator.sol";
 import {Client} from "../../src/Client.sol";
 import {DataCapTypes} from "filecoin-solidity/v0.8/types/DataCapTypes.sol";
-import {CommonTypes} from "filecoin-solidity/v0.8/types/CommonTypes.sol";
 
-contract ReentrantValidatorMock is IValidator {
+contract ReentrantMetaAllocatorMock is IMetaAllocator {
     Client public client;
     DataCapTypes.TransferParams public attackParams;
     uint256 public attackDealId;
@@ -20,14 +19,7 @@ contract ReentrantValidatorMock is IValidator {
         shouldAttack = true;
     }
 
-    function updateLockupPeriod(uint256, uint256) external override {
-        if (shouldAttack) {
-            shouldAttack = false;
-            client.transfer(attackParams, attackDealId, false);
-        }
-    }
-
-    function setDealEndEpoch(uint256, CommonTypes.ChainEpoch) external override {
+    function addVerifiedClient(bytes calldata, uint256) external override {
         if (shouldAttack) {
             shouldAttack = false;
             client.transfer(attackParams, attackDealId, false);

test/contracts/ValidatorMock.sol

@@ -3,10 +3,9 @@
 
 pragma solidity =0.8.30;
 
-import {IValidator} from "../../src/interfaces/Validator.sol";
 import {CommonTypes} from "filecoin-solidity/v0.8/types/CommonTypes.sol";
 
-contract ValidatorMock is IValidator {
+contract ValidatorMock {
     function updateLockupPeriod(uint256 railId, uint256 newLockupPeriod) external {
         // noop
     }