Good idea, bad design: How the Diamond standard falls short
TL;DR: 我们审计了Diamond标准提案的合约可升级性实现，目前无法推荐该方案——但请参考我们的建议和升级策略指南。
我们最近审计了Diamond标准代码的实现，这是一种新的可升级模式。这是一个值得称赞的尝试，但Diamond提案和实现引发了许多担忧。代码过度工程化，包含许多不必要的复杂性，我们目前无法推荐它。
当然，该提案仍处于草案阶段，有成长和改进的空间。一个可行的可升级标准应包括：
- 清晰简单的实现。标准应易于阅读，以简化与第三方应用的集成。
- 完整的升级程序检查清单。升级是一个高风险过程，必须彻底解释。
- 针对最常见可升级错误的链上缓解措施，包括函数遮蔽和冲突。一些错误虽然容易检测，但可能导致严重问题。参见slither-check-upgradeability了解许多可缓解的陷阱。
- 相关风险列表。可升级性很困难；它可能隐藏安全考虑或暗示风险微不足道。EIP是以太坊改进提案，不是商业广告。
- 与最常见测试平台集成的测试。测试应突出显示如何部署系统、如何升级新实现以及升级可能失败的方式。
不幸的是，Diamond提案未能解决这些问题。这太糟糕了，因为我们希望看到一个可升级标准能够解决或至少缓解可升级合约的主要安全陷阱。本质上，标准编写者必须假设开发人员会犯错，并旨在构建一个减轻这些错误的标准。
尽管如此，从Diamond提案中仍有很多值得学习的地方。请继续阅读以了解：
- Diamond提案的工作原理
- 我们的审查揭示了什么
- 我们的建议
- 可升级性标准最佳实践
Diamond提案范式
Diamond提案是EIP 2535中定义的一个进行中的工作。草案声称提出了一种基于delegatecall的合约可升级性新范式。（仅供参考，这里概述了可升级性如何工作。）EIP 2535提议使用：
查找表
基于delegatecall的可升级性主要使用两个组件——代理和实现：
用户与代理交互，代理delegatecall到实现。实现代码被执行，而存储保留在代理中。
使用查找表允许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 }
}
|
顺便说一下，什么是"diamond"？
EIP 2535引入了"diamond术语"，其中"diamond"意思是代理合约，“facet"意思是实现，等等。不清楚为什么引入这个术语，特别是因为可升级性的标准术语是众所周知和定义的。如果您阅读提案，这里有一个关键帮助您翻译：
| Diamond词汇 |
通用名称 |
| Diamond |
代理 |
| Facet |
实现 |
| Cut |
升级 |
| Loupe |
委托函数列表 |
| Finished diamond |
不可升级 |
| Single cut diamond |
移除可升级性函数 |
审计发现和建议
我们对diamond实现的审查发现：
- 代码过度工程化，包含几个 misplaced 优化
- 使用存储指针有风险
- 代码库有函数遮蔽
- 合约缺乏存在性检查
- diamond术语增加了不必要的复杂性
过度工程化的代码
虽然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
|
interface IDiamondLoupe {
/// 这些函数预计会被工具频繁调用
struct Facet {
address facetAddress;
bytes4[] functionSelectors;
}
/// @notice 获取所有facet地址及其四字节函数选择器。
/// @return facets_ Facet
function facets() external view returns (Facet[] memory facets_);
/// @notice 获取特定facet支持的所有函数选择器。
/// @param _facet facet地址。
/// @return facetFunctionSelectors_
function facetFunctionSelectors(address _facet) external view returns (bytes4[] memory facetFunctionSelectors_);
/// @notice 获取diamond使用的所有facet地址。
/// @return facetAddresses_
function facetAddresses() external view returns (address[] memory facetAddresses_);
/// @notice 获取支持给定选择器的facet。
/// @dev 如果未找到facet，返回address(0)。
/// @param _functionSelector 函数选择器。
/// @return facetAddress_ facet地址。
function facetAddress(bytes4 _functionSelector) external view returns (address facetAddress_);
|
这里，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
|
// 添加或替换函数
if (newFacet != 0) {
// 添加和替换选择器
for (uint selectorIndex; selectorIndex < numSelectors; selectorIndex++) {
bytes4 selector;
assembly {
selector := mload(add(facetCut,position))
}
position += 4;
bytes32 oldFacet = ds.facets[selector];
// 添加
if(oldFacet == 0) {
// 在函数结束时更新最后一个插槽
slot.updateLastSlot = true;
ds.facets[selector] = newFacet | bytes32(selectorSlotLength) << 64 | bytes32(selectorSlotsLength);
// 清除插槽中的选择器位置并添加选择器
slot.selectorSlot = slot.selectorSlot & ~(CLEAR_SELECTOR_MASK >> selectorSlotLength * 32) | bytes32(selector) >> selectorSlotLength * 32;
selectorSlotLength++;
// 如果插槽已满，则将其写入存储
if(selectorSlotLength == 8) {
ds.selectorSlots[selectorSlotsLength] = slot.selectorSlot;
slot.selectorSlot = 0;
selectorSlotLength = 0;
selectorSlotsLength++;
}
}
// 替换
else {
require(bytes20(oldFacet) != bytes20(newFacet), "Function cut to same facet.");
// 替换旧的facet地址
ds.facets[selector] = oldFacet & CLEAR_ADDRESS_MASK | newFacet;
}
}
}
|
为了优化此函数的gas效率付出了很多努力。但是升级合约很少进行，所以无论其gas成本如何，它永远不会是一个昂贵的操作。
在另一个不必要的复杂性示例中，使用位操作而不是结构：
1
2
3
4
5
6
|
uint selectorSlotsLength = uint128(slot.originalSelectorSlotsLength);
uint selectorSlotLength = uint128(slot.originalSelectorSlotsLength >> 128);
// uint32 selectorSlotLength, uint32 selectorSlotsLength
// selectorSlotsLength是selectorSlots中32字节插槽的数量。
// selectorSlotLength是selectorSlots最后一个插槽中选择器的数量。
uint selectorSlotsLength;
|
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{
// 合约代表两个实现A和B
// A有一个嵌套结构
// A和B有不同的基础存储指针
// 然而在B中写入将导致写入A变量
// 这是因为B的基础存储指针
// 与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;
}
}
|
在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{
// 添加my_targeted_function()
// 作为委托函数
}
function my_targeted_function() public{
}
fallback () external payable{
// 委托给实现
}
}
|
尽管这个问题是众所周知的，并且代码经过了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)}
}
}
|
我们的建议
- 调用任意合约时，始终检查合约是否存在。
- 如果gas成本是一个问题，仅在调用不返回数据时执行此检查，因为相反的结果意味着执行了一些代码。
不必要的Diamond术语
如前所述，Diamond提案严重依赖其新创建的词汇。这容易出错，使审查更加困难，并且对开发人员没有好处。
diamond是使用其facet中的函数执行函数调用的合约。diamond可以有一个或多个facet。
facet一词来自钻石行业。它是钻石的一个侧面或平面。钻石可以有许多facet。在这个标准中，facet是一个包含一个或多个函数的合约，执行diamond的功能。
loupe是用于观察钻石的放大镜。在这个标准中，loupe是一个提供函数来查看diamond及其facet的facet。
我们的建议
使用常见、众所周知的词汇，不要在不需要时发明术语。
Diamond提案是死胡同吗？
如前所述，我们仍然相信社区将从标准化的可升级模式中受益。但当前的Diamond提案不符合预期的安全要求，也没有比自定义实现带来足够的好处。
然而，该提案仍是一个草案，可能演变成更简单更好的东西。即使没有，一些使用的现有技术，如查找表和任意存储指针，值得继续探索。
那么……可升级性可行吗？
多年来，我们审查了许多可升级合约，并发表了多篇关于此主题的分析。可升级性困难、容易出错并增加风险，我们仍然通常不推荐它作为解决方案。但需要在合约中实现可升级性的开发人员应：
- 考虑不需要delegatecall的可升级性设计（参见Gemini实现）
- 彻底审查现有解决方案及其局限性：
- 合约升级反模式
- 合约迁移如何工作
- 使用OpenZeppelin的可升级性
- 使用crytic.io，或将slither-check-upgradeability添加到您的CI中
如果您对升级策略有任何疑问，请联系我们。我们随时准备提供帮助！