c4-bot-2 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibBytes.sol#L42

Vulnerability details

Impact

The LibBytes::readProposalData function uses inline assembly for efficient data extraction from a byte array. The colRedeemed variable, which represents an 11-byte value within the ProposalData structure, is intended to be extracted by applying a mask to isolate the relevant bytes. However, the current implementation incorrectly uses the add operation. That leads to retrieve incorrect value of colRedeemed variable:


    function readProposalData(address SSTORE2Pointer, uint8 slateLength) internal view returns (MTypes.ProposalData[] memory) {
        bytes memory slate = SSTORE2.read(SSTORE2Pointer);
        // ProposalData is 51 bytes
        require(slate.length % 51 == 0, "Invalid data length");

        MTypes.ProposalData[] memory data = new MTypes.ProposalData[](slateLength);

        for (uint256 i = 0; i < slateLength; i++) {
            // 32 offset for array length, mulitply by each ProposalData
            uint256 offset = i * 51 + 32;

            address shorter; // bytes20
            uint8 shortId; // bytes1
            uint64 CR; // bytes8
            uint88 ercDebtRedeemed; // bytes11
            uint88 colRedeemed; // bytes11

            assembly {
                // mload works 32 bytes at a time
                let fullWord := mload(add(slate, offset))
                // read 20 bytes
                shorter := shr(96, fullWord) // 0x60 = 96 (256-160)
                // read 8 bytes
                shortId := and(0xff, shr(88, fullWord)) // 0x58 = 88 (96-8), mask of bytes1 = 0xff * 1
                // read 64 bytes
                CR := and(0xffffffffffffffff, shr(24, fullWord)) // 0x18 = 24 (88-64), mask of bytes8 = 0xff * 8

                fullWord := mload(add(slate, add(offset, 29))) // (29 offset)
                // read 88 bytes
                ercDebtRedeemed := shr(168, fullWord) // (256-88 = 168)
                // read 88 bytes
@>              colRedeemed := add(0xffffffffffffffffffffff, shr(80, fullWord)) // (256-88-88 = 80), mask of bytes11 = 0xff * 11
            }

            data[i] = MTypes.ProposalData({
                shorter: shorter,
                shortId: shortId,
                CR: CR,
                ercDebtRedeemed: ercDebtRedeemed,
                colRedeemed: colRedeemed
            });
        }

        return data;
    }

The add operation would incorrectly add the mask to the shifted value, potentially resulting in an incorrect value for colRedeemed. The correct operation should use and to apply the mask and isolate the 11-byte colRedeemed value.

The RedemptionFacet contract calls the LibBytes::readProposalData function and uses colRedeemed variable in claimRedemption function.

Proof of Concept

Link to the code: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibBytes.sol#L42

The following contract Assembly is a simple contract that contains two functions: incorrectColRedeemed with the logic from the LibBytes contract and the correctColRedeemed with the correct logic:


// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

contract Assembly {
    function incorrectColRedeemed(uint256 fullWord) public pure returns (uint88) {
        uint88 col;
        assembly {
            col := add(0xffffffffffffffffffffff, shr(80, fullWord))
        }
        return col;
    }

    function correctColRedeemed(uint256 fullWord) public pure returns (uint256) {
        uint88 col;
        assembly {
            col := and(0xffffffffffffffffffffff, shr(80, fullWord))
        }
        return col;
    }
}

The following test file contains test function test_assembly that compares the returned value from the both functions and shows the differences between the results.

contract TestAssembly is Test {

    Assembly asm = new Assembly();

    uint256 testFullWord = 0x1234599990abcdef1234567890abcdef1234567890abcdef1234567890abcdef;

    function test_assembly() external view {
        assert(asm.incorrectColRedeemed(testFullWord) < asm.correctColRedeemed(testFullWord));
        console.log('Incorrect:', asm.incorrectColRedeemed(testFullWord));
        console.log('Correct: ', asm.correctColRedeemed(testFullWord));
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Replace the add operation with an and operation to correctly apply the mask:

colRedeemed := and(0xffffffffffffffffffffff, shr(80, fullWord))

Assessed type

Other

c4-pre-sort commented 7 months ago

raymondfam marked the issue as primary issue

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

raymondfam commented 7 months ago

Will let sponsor to access its validity.

c4-sponsor commented 7 months ago

ditto-eth (sponsor) confirmed

c4-judge commented 7 months ago

hansfriese marked the issue as satisfactory