Hub/Spokes Library V1 - Concrete

The function encodeFeesAreBorrowedAndCreditInterestsAccrueOnUser is intended to encode two uint8 values,feesAreBorrowed and creditInterestsAccrueOnUser, into a single uint256 by placing them in separate bytes. The feesAreBorrowed should occupy the least significant byte, and creditInterestsAccrueOnUser should be shifted left by 8 bits to occupy the second byte.

    function encodeFeesAreBorrowedAndCreditInterestsAccrueOnUser(
        uint8 feesAreBorrowed,
        uint8 creditInterestsAccrueOnUser
    ) internal pure returns (uint256) {
        return uint256(feesAreBorrowed) | (creditInterestsAccrueOnUser) << 8;
    }

However the shift operation is not executed as intended. This is becauses the bitwise OR operation occurs before the shift, resulting in the creditInterestsAccrueOnUser value being encoded incorrectly then shifted, which leads to improper encoding and decodeCreditInterestsAccrueOnUser check always returns false.

    function test_encodeFeesAreBorrowedAndCreditInterestsAccrueOnUser() public pure{
        for(uint256 i; i<type(uint8).max; i++){
            uint256 encoded = Uint8CodecLib.encodeFeesAreBorrowedAndCreditInterestsAccrueOnUser(1, 1);
            assertNotEq(Uint8CodecLib.decodeCreditInterestsAccrueOnUser(encoded), true);
        }
    }

Modify the encoding function to enforce the correct order of operations by adding explicit parentheses and appropriate type casting:

return uint256(feesAreBorrowed) | (uint256(creditInterestsAccrueOnUser) << 8)

SOLVED: The Concrete team fixed the issue. The function was removed from the contract.

The getTrancheAmountFractionInWad and getTrancheFeeFractionInWad functions from ProtectionLibV1 aims to extract a fraction value associated with a specific tranche from a packed uint256 encoding. They both uses the same condition to validate the tranche.

        if ((tranche < 1) && (tranche > numberOfTranches)) revert Errors.InvalidTrancheNumber(tranche);

The condition uses the logical AND operator (&&), which means the revert statement will only be executed if both conditions are true simultaneously. A tranche number cannot be both less than 1 and greater than numberOfTranches at the same time. These two conditions are mutually exclusive.

As a result, invalid tranche numbers that are either less than 1 or greater than numberOfTranches will not trigger the revert, potentially leading to incorrect calculations or out-of-bounds errors later in the function.

The correct logical operator to use in this context is the OR operator (||):

if ((tranche < 1) || (tranche > numberOfTranches)) revert Errors.InvalidTrancheNumber(tranche);

SOLVED: The Concrete team fixed the issue as recommended.

The function encodeProtectionDataFromAbsoluteValues and encodeProtectionData from ProtectionLibV1 contract, currently checks individual tranche amounts and fees to ensure they do not exceed the promisedAmountInBase.

    function encodeProtectionDataFromAbsoluteValues(
        uint256 promisedAmountInBase,
        ProtectionDataAbsolute memory protectionData
    ) internal pure returns (uint256) {
        if (protectionData.numberOfTranches > 3) revert Errors.NumberOfProtectionClaimsTooHigh();
        if (protectionData.ltvProtectForClaimsInBP > MILLION) revert Errors.FractionExceedsUnityInBP();
        if (protectionData.ltvProtectForForeclosureInBP > MILLION) revert Errors.FractionExceedsUnityInBP();
        if (protectionData.openingFeeInBase > promisedAmountInBase) {
            revert Errors.OpeningFeeExceedsPromisedAmount();
        }
        if (protectionData.cancellationFeeInBase > promisedAmountInBase) {
            revert Errors.CancellationFeeExceedsPromisedAmount();
        }
        if (protectionData.trancheAmountInBase.length != protectionData.numberOfTranches) {
            revert Errors.InvalidTrancheNumber(uint8(protectionData.trancheAmountInBase.length));
        }
        if (protectionData.trancheFeeInBase.length != protectionData.numberOfTranches) {
            revert Errors.InvalidTrancheNumber(uint8(protectionData.trancheFeeInBase.length));
        }
        for (uint8 i = 0; i < protectionData.numberOfTranches; i++) {
            if (protectionData.trancheAmountInBase[i] > promisedAmountInBase) {
                revert Errors.TrancheAmountExceedsPromisedAmount(i + 1);
            }
            if (protectionData.trancheFeeInBase[i] > promisedAmountInBase) {
                revert Errors.TrancheFeeExceedsPromisedAmount(i + 1);
            }

    function encodeProtectionData(ProtectionData memory protectionData) internal pure returns (uint256) {
        if (protectionData.numberOfTranches > 3) revert Errors.NumberOfProtectionClaimsTooHigh();
        if (protectionData.ltvProtectClaims > MILLION) revert Errors.FractionExceedsUnityInMillionth();
        if (protectionData.ltvProtectForeclosure > MILLION) revert Errors.FractionExceedsUnityInMillionth();
        if (protectionData.openingFeeRate > MILLION) revert Errors.FractionExceedsUnityInMillionth();
        if (protectionData.cancellationFeeRate > MILLION) revert Errors.FractionExceedsUnityInMillionth();
        if (protectionData.trancheAmountFractionInMillionth.length != protectionData.numberOfTranches) {
            revert Errors.InvalidTrancheNumber(uint8(protectionData.trancheAmountFractionInMillionth.length));
        }
        if (protectionData.trancheFeeRateInMillionth.length != protectionData.numberOfTranches) {
            revert Errors.InvalidTrancheNumber(uint8(protectionData.trancheFeeRateInMillionth.length));
        }
        for (uint8 i = 0; i < protectionData.numberOfTranches; i++) {
            if (protectionData.trancheAmountFractionInMillionth[i] > MILLION) {
                revert Errors.FractionExceedsUnityInMillionth();
            }

However, it fails to verify whether the cumulative sum of all tranche amounts exceeds the promisedAmountInBase. This oversight could allow the total amount of tranches to surpass the promised amount, leading to potential discrepancies in the contracts logic.

    function test_tranchesShouldNoGreaterPromisedAmount() external {
        uint256 promisedAmountInCollateral = 100 ether;
        
        uint256[] memory trancheAmountInCollateral = new uint256[](3);
        uint256[] memory trancheFeeInCollateral = new uint256[](3);

        trancheAmountInCollateral[0] = 100 ether;
        trancheAmountInCollateral[1] = 100 ether;
        trancheAmountInCollateral[2] = 100 ether;

        trancheFeeInCollateral[0] = 10 ether;
        trancheFeeInCollateral[1] = 10 ether;
        trancheFeeInCollateral[2] = 10 ether;

        ProtectionDataAbsolute memory protectionDataAbsolute = ProtectionDataAbsolute({
            endTime: uint40(block.timestamp + DURATION), 
            numberOfTranches: 3,
            protocolRights: 0,
            remoteProxyRights: 0,
            remoteConcreteerRights: 0,
            remotePublicRights: 0,
            ltvProtectForClaimsInBP: concreteClaimBufferInBP, 
            ltvProtectForForeclosureInBP: concreteForeclosureBufferInBP,
            openingFeeInBase: 1 ether,  
            cancellationFeeInBase: 1 ether,
            trancheAmountInBase: trancheAmountInCollateral,
            trancheFeeInBase: trancheFeeInCollateral
        });
        //This should revert
        ProtectionLibV1.encodeProtectionDataFromAbsoluteValues(promisedAmountInCollateral, protectionDataAbsolute);
    }

It is recommended to add checks to validate the cumulative sum of all tranche amounts does not exceed the promisedAmountInBase.

SOLVED: The Concrete team resolved the issue as recommended.

The encodeRightsFromFlags function aims to encode four boolean flags into specific bits within a uint8 value.

    function encodeRightsFromFlags(
        bool protocolCanIntervene,
        bool remoteProxyCanIntervene,
        bool remoteConcreteerCanIntervene,
        bool remotePublicCanIntervene
    ) internal pure returns (uint8) {
        return (protocolCanIntervene ? 0 : 3) | (remoteProxyCanIntervene ? (3 << 2) : 0)
            | (remoteConcreteerCanIntervene ? (3 << 4) : 0) | (remotePublicCanIntervene ? (3 << 6) : 0);
    }

However, the ternary operations are applied inconsistently:

• For protocolCanIntervene, when the flag is true, it sets the bits to 0 - when false, it sets them to 3.

• For the other flags, when the flag is true, it sets the bits to shifted 3 - when false, it sets them to 0.

  
  

This reversal of logic causes the bits to be incorrectly set, leading to potential misinterpretation of access rights.

Consider adjusting the function to use consistent logic across all flags.

ACKNOWLEDGED: The Concrete team acknowledged the issue. This is intention. The default rights setting ought to be that the rights for the protocol are enabled, but for the others not. Since in the EVM by default every uint256 value (such as the protocol Info encoding) is zero, it would be great to reflect the default rights encoding also in this case, e.g. where no protection exists (i.e. the protocol Info = 0). The team solved this issue by reversing the rights encoding for the protocol vs. the other parties. For the protocol, the default is that it can close and foreclose and claim and reclaim.