Chainlink CBOR Audit

The CBOR project package.json lists @ensdomains/buffer v0.0.13 as a dependency, which is outdated. The latest version of the Buffer.sol library fixes two important issues in Buffer::writeUint8:

• The capacity was not properly computed in case of a resize
• The buffer’s length was updated when a byte was being appended at the end of the buffer, but not when written at any larger offset.

In fact, we discovered these bugs while auditing the CBOR contract, and only later realized that they were already fixed by very recent commits in the Buffer repository.

Note that these issues do not affect the current CBOR code, but we still recommend updating to the latest version, to avoid future versions of CBOR becoming vulnerable.

The Buffer library performs some really low-level manipulations of the Solidity memory. Most critically, it manually updates the Solidity Free Memory Pointer (FMP) whenever new space needs to be allocated, incrementing it by the capacity of the newly allocated buffer. However, this update is done entirely inside an inline assembly block, which means that no overflow checks are done.

As a consequence, if a contract calls Buffer::init(buf, capacity), in a context where capacity can be controlled by the adversary, the FMP will be updated to FMP+capacity+32. This value might overflow the uint256 range, with two major consequences:

• the FMP will now point to allocated memory, causing unpredictable behavior
• The buffer’s content will overlap with previously allocated memory, possibly allowing the adversary to manipulate sensitive memory content via the buffer.

Note that the same vulnerability can be exploited via the write* methods, even if Buffer::init itself is not controllable by the adversary, if the offset parameter is user-controllable. Consider, for instance, the following code:

function write(buffer memory buf, uint off, bytes memory data, uint len) internal pure returns(buffer memory) {

    	if (off + len > buf.capacity) {
        	resize(buf, max(buf.capacity, len + off) * 2);
    	}

It could be that the expression (off + len) * 2 does not overflow, but when the resize happens, FMP + (off + len) * 2 does overflow!

We strongly recommend checking for free memory pointer overflows in Buffer::init.

In Buffer.sol, the write functions allow writing at an offset that is greater than the length of the current buffer. This is handled by simply updating the buffer’s length, without initializing the part of the buffer up to that offset, as shown in the code below (from Buffer::write).

if gt(add(len, off), buflen) {
    mstore(bufptr, add(len, off))
}

Note that the memory contents in solidity are not guaranteed to be zeroed (since previous code might have used the part after the FMP as temporary scratchpad). As a consequence, the data in the range buf[old_length:offset] are never initialized, and may potentially contain garbage data.

It is recommended that this range in the buffer be zero initialized.

The CBOR library uses a low-level API that puts the burden of generating valid CBOR to the user. There are numerous ways of generating invalid CBOR via the library:

• Starting but not ending arrays/maps
• Invalid nesting of arrays/maps
• Maps with odd number of elements
• etc

Depending on the use-case, it might be preferable to use a higher-level API that enforces correct CBOR output (or a way to test that the output is valid).

Although the CBOR specification allows for fixed-length arrays and maps, these are not supported by the CBOR library. Extending CBOR::startArray/startMap to support them would be trivial, and likely useful for several applications.

In particular, the array in Chainlink::addStringArray has known length. Using a fixed length array would make the CBOR one byte shorter, and also save the self.buf.endSequence(); call.

In Buffer::write(buffer buf, uint off, bytes* data, uint len), the subexpression off + len appears multiple times throughout the body of both write functions.

Calculating it once and storing it in a local variable could help save some gas and also help with readability.

In Buffer::write(buffer, uint, bytes, uint), there is a redundant max operation:

function write(..) {
    if (off + len > buf.capacity) {
        // Dedaub: Max operation is redundant; will always be (len + off)
        resize(buf, max(buf.capacity, len + off) * 2);
    }
    ...
}

Replacing the operation with just (len + off), or a local variable with that value (as per the A1 suggestion) would help save some gas. The same issue also appears in Buffer::writeUint8.

In CBOR.sol, the pragma solidity directive ranges from version 0.4.19 up to (excluding) 0.9.0. However, Buffer.sol has pragma solidity ^0.8.4; , which makes it impossible to compile CBOR with any Solidity version lower than 0.8.4.

It is recommended that the CBOR directive be updated to have 0.8.4 as a lower bound.

The contracts were compiled with the Solidity compiler v0.8.7 which, at the time of writing, have some known bugs. We inspected the bugs listed for version 0.8.7 and concluded that the subject code is unaffected.