慢雾深度解析 Bitfinex 天价手续费转账:BUG+显示错误「酿成大错」

慢雾科技

    撰文:Thinking @ 慢雾安全团队
    事件背景
    分析源自一笔转账金额 10w USDT,手续费却高达 7,676 枚 ETH 的天价手续费交易。
    https://etherscan.io/tx/0x2c9931793876db33b1a9aad123ad4921dfb9cd5e59dbb78ce78f277759587115
    
    核心点
    技术层面的核心问题是: ethjs-util 的 intToBuffer 不支持传入浮点型的数据,ethereumjs 用了 ethjs-util 的 intToBuffer 。
    简而言之:DApp 在使用 ethereumjs 构造交易的时候,传入的手续费如果带有小数的,会因为类型转换出现 bug 导致在浏览器返回了一个很大的数作为手续费。且硬件钱包没有显示清楚,导致用户直接授权签名了这边天价手续费的交易。
    关键代码分析
    根据此 Issue
    https://github.com/ethereumjs/ethereumjs-monorepo/issues/1497 中的描述,开始了分析。
    我们以倒叙的方式来说明问题,这样更方便理解。核心问题是 ethjs-utilintToBuffer 不支持传入浮点型的数据。
    首先看看关键的代码,提得比较多的是 ethereumjs 的问题,主要聚焦讨论的是 maxPriorityFeePerGasmaxFeePerGas 这两个参数的值,由于传入的浮点型,导致计算错误,得到了错的手续,从而发生了 「天价手续的事件」。
    经过分析后,这两个参数都是经过 toBuffer 进行处理的,所以开始分析 toBuffer
    https://github.com/ethereumjs/ethereumjs-monorepo/blob/cf95e04c6a/packages/tx/hide/eip1559Transaction.ts#L200-L201
    this.maxFeePerGas = new BN(toBuffer(maxFeePerGas === '' ? '0x' : maxFeePerGas)) this.maxPriorityFeePerGas = new BN( toBuffer(maxPriorityFeePerGas === '' ? '0x' : maxPriorityFeePerGas) )
    toBuffer 会去调用 ethjs-utilintToBuffer 函数,这个函数主要处理了两件事情。
    https://github.com/ethjs/ethjs-util/blob/e9aede6681/dist/ethjs-util.js#L1950
    function intToBuffer(i) { var hex = intToHex(i); return new Buffer(padToEven(hex.slice(2)), 'hex');}
    
  1. 将 int 转成 Hex

    https://github.com/ethjs/ethjs-util/blob/e9aede6681/dist/ethjs-util.js#L1939
    function intToHex(i) { var hex = i.toString(16); // eslint-disable-line return '0x' + hex;}
    
  1. 判断是否可以被 2 整除,如果不行需要在字符开头添加一个 0 ,这里主要是为了能够成功的将数据 2 个 1 组写入到 buffer。

    https://github.com/ethjs/ethjs-util/blob/e9aede6681/dist/ethjs-util.js#L1920
    function padToEven(value) { var a = value; // eslint-disable-line if (typeof a !== 'string') { throw new Error('[ethjs-util] while padding to even, value must be string, is currently ' + typeof a + ', while padToEven.'); } if (a.length % 2) { a = '0' + a; } return a;}
    以出错的示例数据:33974229950.550003 进行分析,经过 intToBuffer 函数中的 intToHexpadToEven 处理后得到 7e9059bbe.8ccd,这部分浏览器 js 和 nodejs 的结果都是一致的。
    不一致的地方是在 new Buffer 的操作:
    new Buffer(padToEven(hex.slice(2)), 'hex');
    处理方式分析:浏览器 js
    通过 webpack 打包好 js 文件并对文件进行引用,然后在浏览器上进行调试分析。
    首先输入的示例字符 33974229950.550003 会进入到 intToBuffer 的函数中进行处理。
    
    同步分析 intToBuffer 的处理过程,这部分和」关键代码分析「部分的代码逻辑是一样的,处理转换部分得到的结果是 7e9059bbe.8ccd
    
    接下来分析如何将转换后的字符填充进入的 buffer 中,通过这步可以得到 buffer 的内容是 126, 144, 89, 187, 14, 140, 205 对应的是 7e, 90, 59, bb, e, 8c, cd
    
    > 0x7e -> 126> 0x90 -> 144> 0x59 -> 89> 0xbb -> 187> 0xe -> 14> 0x8c -> 140> 0xcd -> 205
    这里发现 e. 这部分的小数点消失了,于是开始解小数点消失之迷,追踪到 hexWrite 这个函数,这个函数会将得到的数据 2 个一组进行切分。然后用了 parseInt 对切分后的数据进行解析。
    然而 parseInt('e.',16) -> 14===parseInt('e',16) -> 14 消失的小数点被 parseInt 吃掉了,导致最终写入到 buffer 中的数据发生了错误,写入 buffer 的值是 7e9059bbe8ccd
    
    处理方式分析:nodejs
    由于浏览器上出问题的是 7_**__**_e9059bbe.8ccd 在写入 buffer 的时候小数点被 parseInt 吃掉了导致数据出错,但是经过分析,node 的数据也是错误的,且产生错误的原因是和浏览器的不一样。
    首先我们先看下如下的示例:
    node 三组不同的数据填充到 buffer 得到的结果居然是一样的,经过分析 node 的 buffer 有个小特性,就是 2 个一组切分后的数据,如果没法正常通过 hex 解析的,就会把那一组数据以及之后的数据都不处理了,直接返回前面可以被正常处理的那部分数据。可以理解为被截断了。这部分可以参考 node 底层的 buffer 中 node_buffer.cc 中的代码逻辑。
    > new Buffer('7e9059bbe', 'hex') > new Buffer('7e9059bbe.8ccd', 'hex') > new Buffer('7e9059bb', 'hex')
    执行结果的比较
    node 由于会将原始数据 7e9059bbe.8ccd 中的 e. 及之后的数据进行截断,所以最终错误的值是 7e9059bb,相比正确的值 07e9059bbe 小。
    node 的执行结果:
    
    浏览器由于会将原始数据 7e9059bbe.8ccd 中的 . 吃掉,所以最终错误的值是 7e9059bbe8ccd,相比正确的值 07e9059bbe 大很多。
    浏览器的执行结果:
    
    问题的原因
    ethjs-utilintToBuffer 函数不支持浮点型的数据,且在这个函数中没有判断传入的变量类型,来确保变量类型是预期内的。由于 ethereumjstoBuffer 引用了 ethjs-utilintToBuffer 进行处理,也没有对数据进行检查。导致了这次事件的发生,所幸最终善良的矿工归还了「天价手续费 7626 ETH」。
    吸取的教训
    从第三方的库的角度来看,在编码过程中应该要遵循可靠的安全的编码规范,在函数的开头要对传入的数据进行合法性的检查,确保数据和代码逻辑是按照预期内执行。
    从库的使用者的角度来看,使用者应该要自行阅读第三方库的开发文档和对接文档,并且也要对代码中接入第三方库的逻辑进行测试,通过构造大量的数据进行测试,确保业务上能够正常按照期望执行,保证高标准的测试用例的覆盖率。
    
参考资料 :
    https://github.com/ethereumjs/ethereumjs-monorepo/issues/1497
    https://blog.deversifi.com/23-7-million-dollar-ethereum-transaction-fee-post-mortem/
    https://www.chainnews.com/news/611706276133.htm