code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-basin/blob/c1b72d4e372a6246e0efbd57b47fb4cbb5d77062/src/libraries/LibBytes16.sol#L80

Vulnerability details

Description

The pump reads historic reserve counts using the readBytes16() function. Bytes are written with two items per slot, in the following code in storeBytes16():

for (uint256 i; i < maxI; ++i) {
    iByte = i * 64;
    assembly {
        sstore(
            add(slot, mul(i, 32)),
            // @audit - double value packing
            add(mload(add(reserves, add(iByte, 32))), shr(128, mload(add(reserves, add(iByte, 64)))))
        )
    }
}

When read into the output reserves array in readBytes16(), it handles reading differently from odd or even index.

if (i & 1 == 1) {
    assembly {
        mstore(
            // store at index i * 32; i = 0 is skipped by loop
            add(reserves, mul(i, 32)),
            sload(add(slot, iByte))
        )
    }
} else {
    assembly {
        mstore(add(reserves, mul(i, 32)), shl(128, sload(add(slot, iByte))))
    }
}

When the index is odd, the entire slot is read into the reserves array, instead of only the upper 16 bytes. Compare it to the load when index is even, where the lower 16 bytes are lifted to the upper 16 bytes and then read (so the new lower 16 bytes are guaranteed to be zero).

As a result, the reserves array is corrupted at the EVM level. The lower 16 bytes hold an irrevant slot value. At the solidity level, since reserves is defined as bytes16[], it will ignore the lower 16 bytes. However, if we were to look to convert the reserves to a bytes32[] or to a uint data type, the corruption will surface to the solidity level. Due to high risk of misusing the reserves array, the issue is regarded as medium severity.

Impact

The reserves array read using LibBytes16 is corrupted. If the numbers are used for pricing related calculation, this could easily lead to miscalculation and loss of funds.

Tools Used

Manual audit

Recommended Mitigation Steps

It is recommended to clear the lower 16 bytes of the storage slot read before storing it in memory.

mstore(
    // store at index i * 32; i = 0 is skipped by loop
    add(reserves, mul(i, 32)),
    shl(128, shr(128,sload(add(slot, iByte))))
)

Assessed type

en/de-code

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

publiuss commented 1 year ago

In an attempt to recreate this bug, I ran the following test:

    function testBytes16DirtyBytes() public {
        uint256 a = type(uint256).max;
        bytes16[] memory b2 = new bytes16[](1);
        assembly {
            sstore(0, a)
            mstore(add(b2, 32), sload(0))
        }
        console.logBytes16(b2[0]);
        console.logBytes32(bytes32(b2[0]));
        console.log(uint256(uint128(b2[0])));
    }

And got the following results:

Logs:
  0xffffffffffffffffffffffffffffffff
  0xffffffffffffffffffffffffffffffff00000000000000000000000000000000
  340282366920938463463374607431768211455

Based on these results, it seems that converting a bytes16 to a bytes32 or a uint256 does not result in the converted value being corrupted.

Without a valid reproducable proof of concept, this report should be considered invalid.

c4-sponsor commented 1 year ago

publiuss marked the issue as sponsor disputed

trust1995 commented 1 year ago

Hi, please view the POC below

contract POC{
    bytes32 SLOT = bytes32(uint(0x123456781234));

    function store(bytes16 a, bytes16[] memory pair) external {
        bytes32 slot = SLOT;
        assembly {
            sstore(slot, add(mload(add(pair, 32)), shr(128, mload(add(pair, 64)))))
        }

    }

    function load() external returns (bytes32[] memory pair) {
        pair = new bytes32[](2);
        bytes32 slot = SLOT;
        assembly {
            mstore(add(pair, 32), sload(slot))
            mstore(add(pair, 64), shl(128, sload(slot)))
        }
    }
}

You can call store("0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", ["0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","0xcccccccccccccccccccccccccccccccc"]) output of load() is: [0xbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbcccccccccccccccccccccccccccccccc,0xcccccccccccccccccccccccccccccccc00000000000000000000000000000000]

Main thing to notice is that the 0xcccc... at the end of the first bytes32 member is corrupted. This is because we read the entire slot without zeroing out the upper bits.

A very similar situation occurs in the Basin contract.

publiuss commented 1 year ago

In this example, the test directly loads the data into a bytes32[]. In the readBytes16 function, the library loads directly into a bytes16[]. Unless someone modifies the function to read into a bytes32[], there is no vulnerability (according to the above POC). For this reason, the report is invalid imo as it would require modifying the source code for there to be a vulnerability.

If there is a POC that demonstates a potentially vulnerability using the readBytes16 function directly, please share.

trust1995 commented 1 year ago

I don't agree it's invalid, because the memory corruption is there and the only saving grace is that right now it's being read as Bytes16, at this point Solidity would cease to look at the last 16 bytes. However, the code could just as easily read it as bytes32 at which point the data would be completely corrupted. Point is, at the EVM level the code is vulnerable and it is actually counting on compiler-level behavior for safeguarding, which is not well-defined.

liveactionllama commented 1 year ago

Just noting for transparency for other wardens, the sponsor had requested a POC, so C4 staff had reached out to the warden directly with that request. That's why the warden has provided more information above, even though it is outside of the post-judging QA phase.

alcueca commented 1 year ago

No proof of failure in current instance, which includes a specific solidity version. Valid as QA.

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)