BeaconProxy Manual Review Findings

BPY-01M: Insecure Memory Array Expansion

Type	Severity	Location
Language Specific		BeaconProxy.sol:L38-L41

Description:

The construction mechanism of a contract with immutable variables entails retaining the immutable variable values in memory until the BeaconProxy::constructor concludes.

The present code will pad the length of the trailingData input argument to ensure the ABI decode operation that ensues succeeds, however, there is no guarantee that the trailing bytes will be zero or in other words that the MLOAD operations that occur under-the-hood of the abi.decode operation will read zero-value bytes.

Impact:

In its present state, the referenced statement will decode the trailingData payload and then assign to the respective immutable slots. As a result, the read operations from the slots beyond the original length of the trailingData are somewhat safe but invalidate memory layout assumptions of the Solidity language.

To ensure the code behaves as expected across compilation versions and to prevent compilation bugs from manifesting, we advise the code to adopt the slot-by-slot approach we advised.

Example:

Copy
22constructor(bytes memory trailingData) {
23    emit Genesis();
24
25    require(trailingData.length <= MAX_TRAILING_DATA_LENGTH, "trailing data too long");
26
27    // Beacon is always the proxy creator; store it in immutable
28    beacon = msg.sender;
29
30    // Store the beacon address in ERC-1967 slot for compatibility with block explorers
31    assembly {
32        sstore(BEACON_SLOT, caller())
33    }
34
35    // Record length as immutable
36    metadataLength = trailingData.length;
37
38    // Pad length with uninitialized memory so the decode will succeed
39    assembly {
40        mstore(trailingData, 128)
41    }
42    (metadata0, metadata1, metadata2, metadata3) = abi.decode(trailingData, (bytes32, bytes32, bytes32, bytes32));
43}

Recommendation:

We advise consecutive if statements to be introduced, reading 32-byte chunks from the trailingData and storing them to their respective metadata prefixed slot in sequence.

Alleviation (fb2dd77a6ff9b7f710edb48e7eb5437e0db4fc1a):

The Euler Finance team evaluated this exhibit and clarified that they acknowledge the behaviour outlined as whatever is written to the metadataX data points beyond the metadataLength is ignored by the appendment performed in consequent calls.

As such, we consider this exhibit safely acknowledged provided that the compiler version utilized for the project does not prevent uninitialized memory reads from occurring.

BPY-02M: Unhandled Misbehaviour of Memory Load Operation

Type	Severity	Location
Logical Fault		BeaconProxy.sol:L56, L61

Description:

The BeaconProxy::fallback function assumes that the IBeacon::implementation() function will always yield a 32-byte payload, however, that may not be the case.

Impact:

A contradiction exists in the codebase presently whereby a failure of the GenericFactory::implementation function is explicitly handled even though it should never occur, while a failure in how it behaves remains unhandled.

In detail, the code will presently load from the 0 memory pointer meaning that the IMPLEMENTATION_SELECTOR will be yielded as the "implementation" incorrectly.

If the code is meant to be compatible with other implementations than the GenericFactory, the returndatasize should be explicitly handled.

If the code is expected to be solely compatible with the GenericFactory, the code should not evaluate whether the staticcall failed as a staticcall to a compiler-generated getter function cannot practically fail unless insufficient gas was supplied in which case the remaining code will fail out-of-gas regardless.

Example:

Copy
56let result := staticcall(gas(), beacon_, 0, 4, 0, 32)
57if iszero(result) {
58    returndatacopy(0, 0, returndatasize())
59    revert(0, returndatasize())
60}
61let implementation := mload(0)

Recommendation:

As a solution, the code should ensure that the returndatasize() of the staticcall is equal to 32 so as to ensure that the memory pointer at 0 has been properly overwritten.

Alternatively, the code should omit handling of the GenericFactory::implementation function call's failure as the code will succeed in all circumstances.

Alleviation (fb2dd77a6ff9b7f710edb48e7eb5437e0db4fc1a):

The code was updated with the latter of our two recommendations as it expects the call fetching the implementation to always succeed, addressing this exhibit.