`StructHash.sol`'s rebalancing and order typehashing have an extra lower keccak hash of the type that's not included in the `TYPEHASH` for these actions themselves leading to errors

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/StructHash.sol#L86-L108 https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/EIP712.sol#L24-L28

Vulnerability details

Proof of Concept

The StructHash.sol library is used to enforce normal hashing of the struct values for different action types and these hashes are to then be used in order core instances of the KrystalDefi protocol.

Going through the library, we can see that whenever a hashing is to be done for any action the helper _hash() function returns the keccakked abi.encoded data, see an example in regards to the AutoCompoundAction_TYPEHASH.

Issue however is that where as this is rightly done in most instances, it seems to be wrongly implemented for rebalance conditioning and normal order typehashing, see https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/StructHash.sol#L86-L108

//@audit below we can see that the value for type needed is specifed as a  `string` and not `bytes32`, this is important to note cause it affects the final typehash gotten
    // keccak256(
    //     "RebalanceCondition(string type,int160 sqrtPriceX96,int64 timeBuffer,TickOffsetCondition tickOffsetCondition,PriceOffsetCondition priceOffsetCondition,TokenRatioCondition tokenRatioCondition)PriceOffsetCondition(uint32 baseToken,uint256 gteOffsetSqrtPriceX96,uint256 lteOffsetSqrtPriceX96)TickOffsetCondition(uint32 gteTickOffset,uint32 lteTickOffset)TokenRatioCondition(int256 lteToken0RatioX64,int256 gteToken0RatioX64)"
    // );
    bytes32 constant RebalanceCondition_TYPEHASH = 0x79a6efb57bb0d511e670abb964181b04730ebe3a5fd187d05341eeb9288deef8;
    struct RebalanceCondition {
        string _type;
        int160 sqrtPriceX96;
        int64 timeBuffer;
        TickOffsetCondition tickOffsetCondition;
        PriceOffsetCondition priceOffsetCondition;
        TokenRatioCondition tokenRatioCondition;
    }
    function _hash(RebalanceCondition memory obj) private pure returns (bytes32) {
        return keccak256(abi.encode(
            RebalanceCondition_TYPEHASH,
            keccak256(bytes(obj._type)), //@audit
            obj.sqrtPriceX96,
            obj.timeBuffer,
            _hash(obj.tickOffsetCondition),
            _hash(obj.priceOffsetCondition),
            _hash(obj.tokenRatioCondition)
        ));
    }

Evidently, the RebalanceCondition_TYPEHASH expects the type to be attached as a string and not bytes32, however as hinted by the @audit tag, the encode keccak hashes the obj._type which then causes the value used to be a bytes32 instead of a string.

Now using any Keccak 256 online checker, we would see that protocol rightly attaches the typehash to the RebalanceCondition, i.e below we query to get the typehash while having the type attached as a string, which returns 0x79a6efb57bb0d511e670abb964181b04730ebe3a5fd187d05341eeb9288deef8:

However if we use the same tool and instead pass in thetype attached as a bytes32 value, we instead get a whole different typehash, i.e 0xd3d30349ea1059164f17733846216e64f2d4a39c3c19b9812099f3d03a3492c6 which showcases that the wrong typehash is being used for the Reblancing conditions and as such would translate to protocol not being compatible with the intended inherited implementation of EIP 712 and cause issues with integrators who correctly sign their orders.

NB: The same bug case is applicable to the `Order_TYPEHASH cause the typehash attached as a constant in the library expects the orderType to be attached as a string and not bytes32.

Impact

As hinted under the Proof of Concept the Rebalancing condition and Order have an incorrect typehash since an extra keccak of the type is being done, which would lead to issues when attempting to vaildate the signatures applied in the snippet below since valid signatures would be assumed as invalid in the in-scope EIP712.sol https://github.com/code-423n4/2024-06-krystal-defi/blob/f65b381b258290653fa638019a5a134c4ef90ba8/src/EIP712.sol#L24-L28

    function recover(StructHash.Order memory order, bytes memory signature) internal view returns (address) {
        bytes32 digest = _hashTypedDataV4(StructHash._hash(order));//@audit
        return ECDSA.recover(digest, signature);
    }

And extensively when validating orders in V3Automation.sol.

Tool Used

• Manual review
• Online keccak256 checker for validating that the hashes are not actually for the correct structures.
• Past findings: 1 & 2 on Code4rena from Q4 2023.

Recommended Mitigation Steps

Modify the structure typehash to point to the correct structures, i.e modify the typehashes to expect a bytes32 for the order type and not a string. So 0xd3d30349ea1059164f17733846216e64f2d4a39c3c19b9812099f3d03a3492c6 should be used for the RebalanceCondition, pseudo fixes:

-    //     "RebalanceCondition(string type,int160 sqrtPriceX96,int64 timeBuffer,TickOffsetCondition tickOffsetCondition,PriceOffsetCondition priceOffsetCondition,TokenRatioCondition tokenRatioCondition)PriceOffsetCondition(uint32 baseToken,uint256 gteOffsetSqrtPriceX96,uint256 lteOffsetSqrtPriceX96)TickOffsetCondition(uint32 gteTickOffset,uint32 lteTickOffset)TokenRatioCondition(int256 lteToken0RatioX64,int256 gteToken0RatioX64)"
+    //     "RebalanceCondition(bytes32 type,int160 sqrtPriceX96,int64 timeBuffer,TickOffsetCondition tickOffsetCondition,PriceOffsetCondition priceOffsetCondition,TokenRatioCondition tokenRatioCondition)PriceOffsetCondition(uint32 baseToken,uint256 gteOffsetSqrtPriceX96,uint256 lteOffsetSqrtPriceX96)TickOffsetCondition(uint32 gteTickOffset,uint32 lteTickOffset)TokenRatioCondition(int256 lteToken0RatioX64,int256 gteToken0RatioX64)"
-    bytes32 constant RebalanceCondition_TYPEHASH = 0x79a6efb57bb0d511e670abb964181b04730ebe3a5fd187d05341eeb9288deef8;
+    bytes32 constant RebalanceCondition_TYPEHASH = 0xd3d30349ea1059164f17733846216e64f2d4a39c3c19b9812099f3d03a3492c6;

And a similar fix should be applied to the Order_TYPEHASH.

Assessed type

Context

[3docSec]

Good quality report, provisionally marking as satisfactory

[c4-judge]

3docSec marked the issue as satisfactory

[c4-judge]

3docSec marked the issue as selected for report

[Haupc]

as EIP712 doc mentioned: The dynamic values bytes and string are encoded as a keccak256 hash of their contents. and also in the doc example bytes32 constant MAIL_TYPEHASH = keccak256( "Mail(address from,address to,string contents)"); this type hash should not be bytes32 since type of field is string

[3docSec]

I agree with the sponsor. The EIP-712 docs make an example where:

bytes32 constant MAIL_TYPEHASH = keccak256("Mail(address from,address to,string contents)");

and then encodes the object hash as follows:

function hashStruct(Mail memory mail) pure returns (bytes32 hash) {
    return keccak256(abi.encode(
        MAIL_TYPEHASH,
        mail.from,
        mail.to,
        keccak256(mail.contents)
    ));
}

which is the same that the highlighted code does: declaring it as string in the TYPEHASH definition, then encoding it as bytes32 output by keccak256 of the string value.

[c4-judge]

3docSec marked the issue as unsatisfactory: Insufficient proof