Strategy can be extended, leading to inconsistent token counts and breaking immutability

[mehdi-defiesta]

Description

The ship function in Aqua.sol is intended to initialize a new, immutable strategy. However, the check for an existing strategy is done on a per-token basis rather than a per-strategy basis. This allows a maker to call ship multiple times with the same strategy data (resulting in the same strategyHash ) but with different lists of tokens. Each subsequent call adds new tokens to the strategy, but with a tokensCount value corresponding to the length of the token array in that specific call, not the total number of tokens in the strategy.

This behavior violates the documented principle of strategy immutability and breaks a core invariant of the protocol (invariant number 5), which states that all tokens of an active strategy should share an identical tokensCount . This inconsistency can mislead developers of applications built on Aqua, as functions like safeBalances do not enforce this invariant, and it complicates the strategy lifecycle for makers, who must dock their strategy in the same piecemeal batches it was shipped in.

Example of the issue:

1. A maker calls ship(app, strategy, [T1, T2], amounts). For this strategyHash , tokens T1 and T2 are stored with tokensCount = 2 .
2. The same maker calls ship again with the same strategy but a new token list: ship(app, strategy, [T3, T4, T5], amounts) .
3. The check require(balance.tokensCount == 0, ...) passes for T3, T4, and T5.
4. Tokens T3, T4, and T5 are added to the same strategyHash , but with tokensCount = 3.
5. The strategy now has five tokens, but with inconsistent tokensCount values (2 for T1/T2, 3 for T3/T4/T5).
6. Attempting to dock all five tokens at once via dock(app, strategyHash, [T1, T2, T3, T4, T5]) will fail
   because the check require(balance.tokensCount == tokens.length, ...) will not hold for any of the tokens
   (e.g., for T1, 2 != 5 ).

// File: src/Aqua.sol
function ship(address app, bytes calldata strategy, address[] calldata tokens, uint256[] calldata amounts) external returns(bytes32 strategyHash) {
   strategyHash = keccak256(strategy);
   uint8 tokensCount = tokens.length.toUint8();
   require(tokensCount != _DOCKED, MaxNumberOfTokensExceeded(tokensCount, _DOCKED));
   emit Shipped(msg.sender, app, strategyHash, strategy);
   for (uint256 i = 0; i < tokens.length; i++) {
     Balance storage balance = _balances[msg.sender][app][strategyHash][tokens[i]];
     // This check is per-token, allowing other tokens to be added to the same strategyHash later.
     require(balance.tokensCount == 0, StrategiesMustBeImmutable(app, strategyHash));
     balance.store(amounts[i].toUint248(), tokensCount);
     emit Pushed(msg.sender, app, strategyHash, tokens[i], amounts[i]);
   }
}

PoC

function test_PoC_MultipleShipsBreakImmutability() public {
        vm.startPrank(maker);
        
        // Create a strategy
        Strategy memory strategy = Strategy({
            maker: maker,
            param1: 100,
            param2: 200
        });
        
        bytes memory strategyBytes = abi.encode(strategy);
        bytes32 expectedHash = keccak256(strategyBytes);
        
        // STEP 1: First ship with token1 and token2
        address[] memory tokens1 = new address[](2);
        tokens1[0] = address(token1);
        tokens1[1] = address(token2);
        
        uint256[] memory amounts1 = new uint256[](2);
        amounts1[0] = 1000e18;
        amounts1[1] = 1000e18;
        
        bytes32 strategyHash1 = aqua.ship(app, strategyBytes, tokens1, amounts1);
        
        // Verify the strategy hash is as expected
        assertEq(strategyHash1, expectedHash, "Strategy hash should match");
        
        // Check tokensCount for token1 and token2
        (uint248 balance1, uint8 tokensCount1) = aqua.rawBalances(maker, app, strategyHash1, address(token1));
        (uint248 balance2, uint8 tokensCount2) = aqua.rawBalances(maker, app, strategyHash1, address(token2));
        
        assertEq(tokensCount1, 2, "Token1 should have tokensCount=2");
        assertEq(tokensCount2, 2, "Token2 should have tokensCount=2");
        assertEq(balance1, 1000e18, "Token1 balance should be 1000e18");
        assertEq(balance2, 1000e18, "Token2 balance should be 1000e18");
        
        // STEP 2: Ship again with SAME strategy but different tokens (token3, token4, token5)
        address[] memory tokens2 = new address[](3);
        tokens2[0] = address(token3);
        tokens2[1] = address(token4);
        tokens2[2] = address(token5);
        
        uint256[] memory amounts2 = new uint256[](3);
        amounts2[0] = 500e18;
        amounts2[1] = 500e18;
        amounts2[2] = 500e18;
        
        // This SHOULD fail if strategies were truly immutable, but it doesn't!
        bytes32 strategyHash2 = aqua.ship(app, strategyBytes, tokens2, amounts2);
        
        // Verify it's the SAME strategy hash
        assertEq(strategyHash2, strategyHash1, "Should be the same strategy hash");
        
        // STEP 3: Check the inconsistent tokensCount values
        (uint248 balance3, uint8 tokensCount3) = aqua.rawBalances(maker, app, strategyHash1, address(token3));
        (uint248 balance4, uint8 tokensCount4) = aqua.rawBalances(maker, app, strategyHash1, address(token4));
        (uint248 balance5, uint8 tokensCount5) = aqua.rawBalances(maker, app, strategyHash1, address(token5));
        
        // These will have tokensCount=3, not 2!
        assertEq(tokensCount3, 3, "Token3 should have tokensCount=3");
        assertEq(tokensCount4, 3, "Token4 should have tokensCount=3");
        assertEq(tokensCount5, 3, "Token5 should have tokensCount=3");
        assertEq(balance3, 500e18, "Token3 balance should be 500e18");
        assertEq(balance4, 500e18, "Token4 balance should be 500e18");
        assertEq(balance5, 500e18, "Token5 balance should be 500e18");
        
        // STEP 4: Demonstrate the broken invariant - same strategy, different tokensCount
        console.log("=== BROKEN INVARIANT DEMONSTRATED ===");
        console.log("Same strategyHash:", uint256(strategyHash1));
        console.log("Token1 tokensCount:", tokensCount1);
        console.log("Token2 tokensCount:", tokensCount2);
        console.log("Token3 tokensCount:", tokensCount3);
        console.log("Token4 tokensCount:", tokensCount4);
        console.log("Token5 tokensCount:", tokensCount5);
        console.log("=====================================");
        
        // The strategy now has 5 tokens total, but with inconsistent tokensCount values!
        // This violates the protocol's immutability guarantee
        
        vm.stopPrank();
    }

  === BROKEN INVARIANT DEMONSTRATED ===
  Same strategyHash: 48978388632646054151115247388432017154095045079847394744681229877849820775720
  Token1 tokensCount: 2
  Token2 tokensCount: 2
  Token3 tokensCount: 3
  Token4 tokensCount: 3
  Token5 tokensCount: 3
  =====================================