diff --git a/.changeset/warm-geese-dance.md b/.changeset/warm-geese-dance.md new file mode 100644 index 000000000..5deb7a3e6 --- /dev/null +++ b/.changeset/warm-geese-dance.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': patch +--- + +`Base64`: Fix issue where dirty memory located just after the input buffer is affecting the result. diff --git a/contracts/mocks/Base64DirtyUpgradeable.sol b/contracts/mocks/Base64DirtyUpgradeable.sol new file mode 100644 index 000000000..b6554786b --- /dev/null +++ b/contracts/mocks/Base64DirtyUpgradeable.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import {Base64Upgradeable} from "../utils/Base64Upgradeable.sol"; +import {Initializable} from "../proxy/utils/Initializable.sol"; + +contract Base64DirtyUpgradeable is Initializable { + struct A { + uint256 value; + } + + function __Base64Dirty_init() internal onlyInitializing { + } + + function __Base64Dirty_init_unchained() internal onlyInitializing { + } + function encode(bytes memory input) public pure returns (string memory) { + A memory unused = A({value: type(uint256).max}); + // To silence warning + unused; + + return Base64Upgradeable.encode(input); + } + + /** + * @dev This empty reserved space is put in place to allow future versions to add new + * variables without shifting down storage in the inheritance chain. + * See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps + */ + uint256[50] private __gap; +} diff --git a/contracts/mocks/WithInit.sol b/contracts/mocks/WithInit.sol index ca74b669b..ab19b91be 100644 --- a/contracts/mocks/WithInit.sol +++ b/contracts/mocks/WithInit.sol @@ -147,6 +147,13 @@ contract Bytes32ArraysMockUpgradeableWithInit is Bytes32ArraysMockUpgradeable { __Bytes32ArraysMock_init(array); } } +import "./Base64DirtyUpgradeable.sol"; + +contract Base64DirtyUpgradeableWithInit is Base64DirtyUpgradeable { + constructor() payable initializer { + __Base64Dirty_init(); + } +} import "./CallReceiverMockUpgradeable.sol"; contract CallReceiverMockUpgradeableWithInit is CallReceiverMockUpgradeable { diff --git a/contracts/utils/Base64Upgradeable.sol b/contracts/utils/Base64Upgradeable.sol index 1ca0a0bfb..473466c33 100644 --- a/contracts/utils/Base64Upgradeable.sol +++ b/contracts/utils/Base64Upgradeable.sol @@ -41,12 +41,19 @@ library Base64Upgradeable { let tablePtr := add(table, 1) // Prepare result pointer, jump over length - let resultPtr := add(result, 32) + let resultPtr := add(result, 0x20) + let dataPtr := data + let endPtr := add(data, mload(data)) + + // In some cases, the last iteration will read bytes after the end of the data. We cache the value, and + // set it to zero to make sure no dirty bytes are read in that section. + let afterPtr := add(endPtr, 0x20) + let afterCache := mload(afterPtr) + mstore(afterPtr, 0x00) // Run over the input, 3 bytes at a time for { - let dataPtr := data - let endPtr := add(data, mload(data)) + } lt(dataPtr, endPtr) { } { @@ -54,13 +61,12 @@ library Base64Upgradeable { dataPtr := add(dataPtr, 3) let input := mload(dataPtr) - // To write each character, shift the 3 bytes (18 bits) chunk + // To write each character, shift the 3 byte (24 bits) chunk // 4 times in blocks of 6 bits for each character (18, 12, 6, 0) - // and apply logical AND with 0x3F which is the number of - // the previous character in the ASCII table prior to the Base64 Table - // The result is then added to the table to get the character to write, - // and finally write it in the result pointer but with a left shift - // of 256 (1 byte) - 8 (1 ASCII char) = 248 bits + // and apply logical AND with 0x3F to bitmask the least significant 6 bits. + // Use this as an index into the lookup table, mload an entire word + // so the desired character is in the least significant byte, and + // mstore8 this least significant byte into the result and continue. mstore8(resultPtr, mload(add(tablePtr, and(shr(18, input), 0x3F)))) resultPtr := add(resultPtr, 1) // Advance @@ -75,6 +81,9 @@ library Base64Upgradeable { resultPtr := add(resultPtr, 1) // Advance } + // Reset the value that was cached + mstore(afterPtr, afterCache) + // When data `bytes` is not exactly 3 bytes long // it is padded with `=` characters at the end switch mod(mload(data), 3) diff --git a/package-lock.json b/package-lock.json index 2ef7afc8f..fe35f78f8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "openzeppelin-solidity", - "version": "4.9.0", + "version": "4.9.5", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "openzeppelin-solidity", - "version": "4.9.0", + "version": "4.9.5", "license": "MIT", "bin": { "openzeppelin-contracts-migrate-imports": "scripts/migrate-imports.js" diff --git a/test/utils/Base64.test.js b/test/utils/Base64.test.js index dfff0b0d0..c5d7add0e 100644 --- a/test/utils/Base64.test.js +++ b/test/utils/Base64.test.js @@ -1,8 +1,9 @@ const { expect } = require('chai'); const Base64 = artifacts.require('$Base64'); +const Base64Dirty = artifacts.require('$Base64Dirty'); -contract('Strings', function () { +contract('Base64', function () { beforeEach(async function () { this.base64 = await Base64.new(); }); @@ -30,4 +31,13 @@ contract('Strings', function () { expect(await this.base64.$encode([])).to.equal(''); }); }); + + it('Encode reads beyond the input buffer into dirty memory', async function () { + const mock = await Base64Dirty.new(); + const buffer32 = Buffer.from(web3.utils.soliditySha3('example').replace(/0x/, ''), 'hex'); + const buffer31 = buffer32.slice(0, -2); + + expect(await mock.encode(buffer31)).to.equal(buffer31.toString('base64')); + expect(await mock.encode(buffer32)).to.equal(buffer32.toString('base64')); + }); });