钻石标准:好想法,坏设计——安全审计揭示的升级性隐患

本文深入分析了EIP-2535钻石标准的实现缺陷,包括过度工程化、存储指针风险、函数遮蔽等问题,并提供了升级性标准的最佳实践建议,帮助开发者规避常见陷阱。

好想法,坏设计:钻石标准的不足之处

TL;DR:我们审计了钻石标准提案的合约可升级性实现,目前无法推荐其现有形式——但请参考我们的建议和升级策略指南。

我们最近审计了钻石标准代码的实现,这是一种新的可升级性模式。这是一项值得称赞的尝试,但钻石提案及其实现引发了许多担忧。代码过度工程化,包含许多不必要的复杂性,目前我们无法推荐它。

当然,该提案仍处于草案阶段,有成长和改进的空间。一个可行的可升级性标准应包括:

  • 清晰简单的实现:标准应易于阅读,以简化与第三方应用的集成。
  • 全面的升级程序清单:升级是一个高风险过程,必须彻底解释。
  • 针对最常见可升级性错误的链上缓解措施:包括函数遮蔽和冲突。尽管一些错误容易检测,但可能导致严重问题。参见slither-check-upgradeability以了解许多可缓解的陷阱。
  • 相关风险列表:可升级性很困难;它可能隐藏安全考虑或暗示风险微不足道。EIP是改进以太坊的提案,不是商业广告。
  • 与最常见测试平台集成的测试:测试应突出显示如何部署系统、如何升级新实现以及升级可能失败的方式。

不幸的是,钻石提案未能解决这些问题。这太糟糕了,因为我们希望看到一个可升级的标准,能够解决或至少缓解可升级合约的主要安全陷阱。本质上,标准编写者必须假设开发人员会犯错误,并旨在构建一个减轻这些错误的标准。

尽管如此,从钻石提案中仍有很多值得学习的地方。请继续阅读以了解:

  • 钻石提案的工作原理
  • 我们的审查揭示了什么
  • 我们的建议
  • 可升级性标准的最佳实践

钻石提案范式

钻石提案是一个进行中的工作,定义在EIP 2535中。草案声称提出了一种基于delegatecall的合约可升级性新范式。(仅供参考,这里是如何实现可升级性的概述。)EIP 2535提议使用:

  • 用于实现的查找表
  • 任意存储指针

查找表

基于delegatecall的可升级性主要使用两个组件——代理和实现:

图1:基于delegatecall的可升级性(单一实现)

用户与代理交互,代理delegatecall到实现。实现代码被执行,而存储保持在代理中。

使用查找表允许delegatecall到多个合约实现,其中根据要执行的函数选择适当的实现:

图2:基于delegatecall的可升级性(多个实现)

这种模式并不新鲜;其他项目过去曾使用此类查找表进行可升级性。参见ColonyNetwork为例。

任意存储指针

提案还建议使用Solidity最近引入的一个特性:任意存储指针,它(如名称所示)允许将存储指针分配到任意位置。

因为存储保持在代理上,实现的存储布局必须遵循代理的存储布局。在进行升级时,跟踪此布局可能很困难(参见此处的示例)。

EIP提议每个实现都有一个关联的结构来保存实现变量,以及一个指向存储结构的任意存储位置的指针。这类似于非结构化存储模式,其中新的Solidity特性允许使用结构而不是单个变量。

假设只要它们各自的基础指针不同,两个不同实现的两个结构就不能冲突。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
bytes32 constant POSITION = keccak256(
    "some_string"
);

struct MyStruct {
    uint var1;
    uint var2;
}

function get_struct() internal pure returns(MyStruct storage ds) {
    bytes32 position = POSITION;
    assembly { ds_slot := position }
}

图3:存储指针示例

图4:存储指针表示

顺便说一下,什么是“钻石”?

EIP 2535引入了“钻石术语”,其中“钻石”一词表示代理合约,“刻面”表示实现,等等。不清楚为什么引入这种术语,特别是因为可升级性的标准术语是众所周知和定义的。如果您阅读提案,这里有一个关键帮助您翻译:

钻石词汇 通用名称
Diamond Proxy
Facet Implementation
Cut Upgrade
Loupe 委托函数列表
Finished diamond 不可升级
Single cut diamond 移除可升级性函数

图5:钻石提案使用新术语来指代现有想法

审计发现和建议

我们对钻石实现的审查发现:

  • 代码过度工程化,包括几个 misplaced 优化
  • 使用存储指针有风险
  • 代码库存在函数遮蔽
  • 合约缺乏存在性检查
  • 钻石术语增加了不必要的复杂性

过度工程化的代码

虽然EIP中提出的模式很简单,但其实际实现难以阅读和审查,增加了问题的可能性。

例如,链上保存的许多数据都很繁琐。虽然提案只需要一个从函数签名到实现地址的查找表,但EIP定义了许多需要存储额外数据的接口:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
interface IDiamondLoupe {
    /// These functions are expected to be called frequently
    /// by tools.

    struct Facet {
        address facetAddress;
        bytes4[] functionSelectors;
    }

    /// @notice Gets all facet addresses and their four byte function selectors.
    /// @return facets_ Facet
    function facets() external view returns (Facet[] memory facets_);

    /// @notice Gets all the function selectors supported by a specific facet.
    /// @param _facet The facet address.
    /// @return facetFunctionSelectors_
    function facetFunctionSelectors(address _facet) external view returns (bytes4[] memory facetFunctionSelectors_);

    /// @notice Get all the facet addresses used by a diamond.
    /// @return facetAddresses_
    function facetAddresses() external view returns (address[] memory facetAddresses_);

    /// @notice Gets the facet that supports the given selector.
    /// @dev If facet is not found return address(0).
    /// @param _functionSelector The function selector.
    /// @return facetAddress_ The facet address.
    function facetAddress(bytes4 _functionSelector) external view returns (address facetAddress_);
}

图6:钻石接口

这里,facetFunctionSelectors返回一个实现的所有函数选择器。此信息仅对离线组件有用,这些组件已经可以从合约的事件中提取信息。在链上不需要这样的特性,特别是因为它显著增加了代码复杂性。

此外,许多代码复杂性是由于在不需要的地方进行优化。例如,用于升级实现的函数应该是直接的。获取一个新地址和签名,它应该更新查找表中的相应条目。好吧,执行此操作的部分函数如下:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
// adding or replacing functions
if (newFacet != 0) {
    // add and replace selectors
    for (uint selectorIndex; selectorIndex < numSelectors; selectorIndex++) {
        bytes4 selector;
        assembly {
            selector := mload(add(facetCut,position))
        }
        position += 4;
        bytes32 oldFacet = ds.facets[selector];
        // add
        if(oldFacet == 0) {
            // update the last slot at then end of the function
            slot.updateLastSlot = true;
            ds.facets[selector] = newFacet | bytes32(selectorSlotLength) << 64 | bytes32(selectorSlotsLength);
            // clear selector position in slot and add selector
            slot.selectorSlot = slot.selectorSlot & ~(CLEAR_SELECTOR_MASK >> selectorSlotLength * 32) | bytes32(selector) >> selectorSlotLength * 32;
            selectorSlotLength++;
            // if slot is full then write it to storage
            if(selectorSlotLength == 8) {
                ds.selectorSlots[selectorSlotsLength] = slot.selectorSlot;
                slot.selectorSlot = 0;
                selectorSlotLength = 0;
                selectorSlotsLength++;
            }
        }
        // replace
        else {
            require(bytes20(oldFacet) != bytes20(newFacet), "Function cut to same facet.");
            // replace old facet address
            ds.facets[selector] = oldFacet & CLEAR_ADDRESS_MASK | newFacet;
        }
    }
}

图7:升级函数

为了优化此函数的燃气效率,付出了很多努力。但升级合约很少进行,因此无论其燃气成本如何,它永远不会是昂贵的操作。

在另一个不必要的复杂性示例中,使用位操作而不是结构:

1
2
3
4
5
6
7
uint selectorSlotsLength = uint128(slot.originalSelectorSlotsLength);
uint selectorSlotLength = uint128(slot.originalSelectorSlotsLength >> 128);
// uint32 selectorSlotLength, uint32 selectorSlotsLength
// selectorSlotsLength is the number of 32-byte slots in selectorSlots.
// selectorSlotLength is the number of selectors in the last slot of
// selectorSlots.
uint selectorSlotsLength;

图8:使用位操作而不是结构

11月5日更新:自我们审计以来,参考实现已更改,但其底层复杂性仍然存在。现在有三个参考实现,这对用户来说更加困惑,对提案的进一步审查也更加困难。

我们的建议

  • 始终力求简单,并尽可能将代码保持在链下。
  • 在编写新标准时,保持代码可读且易于理解。
  • 在实施优化之前分析需求。

存储指针风险

尽管声称如果基础指针不同,冲突是不可能的,但恶意合约可能与另一个实现的变量冲突。基本上,这是因为Solidity存储变量的方式以及影响映射或数组。例如:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
contract TestCollision{
    
    // The contract represents two implementations, A and B
    // A has a nested structure 
    // A and B have different bases storage pointer 
    // Yet writing in B, will lead to write in A variable
    // This is because the base storage pointer of B 
    // collides with A.ds.my_items[0].elems
    
    bytes32 constant public A_STORAGE = keccak256(
        "A"
    );
    
    struct InnerStructure{
        uint[] elems;
    }
    
    struct St_A {
        InnerStructure[] my_items;
    }

    function pointer_to_A() internal pure returns(St_A storage s) {
        bytes32 position = A_STORAGE;
        assembly { s_slot := position }
    }
    
    
    bytes32 constant public B_STORAGE = keccak256(
        hex"78c8663007d5434a0acd246a3c741b54aecf2fefff4284f2d3604b72f2649114"
    );
    
    struct St_B {
        uint val;
    }

    function pointer_to_B() internal pure returns(St_B storage s) {
        bytes32 position = B_STORAGE;
        assembly { s_slot := position }
    }
    
    
    constructor() public{
        St_A storage ds = pointer_to_A();
        ds.my_items.push();
        ds.my_items[0].elems.push(100);
    }
    
    function get_balance() view public returns(uint){
        St_A storage ds = pointer_to_A();
        return ds.my_items[0].elems[0];
    }
    
    function exploit(uint new_val) public{
        St_B storage ds = pointer_to_B();
        ds.val = new_val;
    }
    
}

图9:存储指针冲突

在exploit中,对B_STORAGE基础指针的写入实际上会写入my_items[0].elems[0],这是从A_STORAGE基础指针读取的。恶意所有者可以推送一个看起来良性但包含后门的升级。

EIP没有防止这些恶意冲突的指南。此外,如果指针在删除后被重用,重用将导致数据泄露。

我们的建议

  • 低级存储操作是有风险的,因此在设计依赖它们的系统时要格外小心。
  • 使用带有结构的非结构化存储进行可升级性是一个有趣的想法,但它需要彻底的文档和关于在基础指针中检查什么的指南。

函数遮蔽

可升级合约通常在代理中有函数遮蔽应该被委托的函数。对这些函数的调用将永远不会被委托,因为它们将在代理中执行。此外,相关代码将不可升级。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
contract Proxy {

    constructor(...) public{
          // add my_targeted_function() 
          // as a delegated function
    }
    
    function my_targeted_function() public{
    }
 
    fallback () external payable{
          // delegate to implementations
    }
}

图10:遮蔽问题的简化

尽管这个问题是众所周知的,并且代码经过了EIP作者的审查,但我们在合约中发现了两个函数遮蔽的实例。

我们的建议

  • 在编写可升级合约时,使用crytic.io或slither-check-upgradeability来捕获遮蔽实例。
  • 这个问题突出了一个重要点:开发人员会犯错误。任何新标准都应包括对常见错误的缓解措施,如果它要优于自定义解决方案。

无合约存在性检查

另一个常见错误是缺乏对合约代码的存在性检查。如果代理委托到错误的地址,或已被销毁的实现,对实现的调用将返回成功,即使没有代码被执行(参见Solidity文档)。因此,调用者不会注意到问题,这种行为可能会破坏第三方合约集成。

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
fallback() external payable {
    DiamondStorage storage ds;
    bytes32 position = DiamondStorageContract.DIAMOND_STORAGE_POSITION;
    assembly { ds_slot := position }
    address facet = address(bytes20(ds.facets[msg.sig]));
    require(facet != address(0), "Function does not exist.");
    assembly {
        calldatacopy(0, 0, calldatasize())
        let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0)
        let size := returndatasize()
        returndatacopy(0, 0, size)
        switch result
        case 0 {revert(0, size)}
        default {return (0, size)}
    }
}

图11:无合约存在性检查的回退函数

我们的建议

  • 在调用任意合约时,始终检查合约是否存在。
  • 如果燃气成本是一个问题,仅在调用不返回数据时执行此检查,因为相反的结果意味着执行了一些代码。

不必要的钻石术语

如前所述,钻石提案严重依赖其新创建的词汇。这容易出错,使审查更加困难,并且对开发人员没有好处。

钻石是一个使用其刻面的函数来执行函数调用的合约。一个钻石可以有一个或多个刻面。

刻面一词来自钻石行业。它是钻石的一个侧面或平面。一个钻石可以有许多刻面。在这个标准中,刻面是一个具有一个或多个函数的合约,执行钻石的功能。

放大镜是一种用于观察钻石的放大镜。在这个标准中,放大镜是一个提供函数来查看钻石及其刻面的刻面。

图12:EIP将标准术语重新定义为与软件工程无关的术语

我们的建议: 使用常见、众所周知的词汇,并在不需要时不发明术语。

钻石提案是死胡同吗?

如前所述,我们仍然相信社区将从标准化的可升级性模式中受益。但当前的钻石提案不符合预期的安全要求,也没有带来超过自定义实现的足够好处。

然而,该提案仍处于草案阶段,可能演变成更简单更好的东西。即使没有,一些使用的现有技术,如查找表和任意存储指针,值得继续探索。

那么……可升级性可行吗?

多年来,我们审查了许多可升级合约,并发布了关于此主题的若干分析。可升级性是困难的、容易出错的,并增加风险,我们仍然通常不推荐它作为解决方案。但需要在其合约中实现可升级性的开发人员应:

  • 考虑不需要delegatecall的可升级性设计(参见Gemini实现)
  • 彻底审查现有解决方案及其局限性:
    • 合约升级反模式
    • 合约迁移如何工作
    • 使用OpenZeppelin的可升级性
  • 使用crytic.io,或将slither-check-upgradeability添加到您的CI中

如果您对升级策略有任何疑问,请与我们联系。我们随时准备提供帮助!

comments powered by Disqus
使用 Hugo 构建
主题 StackJimmy 设计