Repository: defi-wonderland/smock
Pull request #161 (2022-09-27 21:31:17)

fix: bug with usage of padNumHexSlotValue
**Description**
In mappings using the function `padNumHexSlotValue` causes a bug on hashes of bytes32 as a key. We should be using the function `padBytesHexSlotValue` which is targeted for bytes values.

A different approach from #160 although it fixes the issue I believe that the usage of the `padNumHexSlotValue` is incorrectly when we deal with `bytes` type as key


Patch:
diff --git a/src/utils/storage.ts b/src/utils/storage.ts
index 19d9116..eb9a6ca 100644
--- a/src/utils/storage.ts
+++ b/src/utils/storage.ts
@@ -252,7 +252,7 @@ function encodeVariable(
       return [
         {
           key: slotKey,
-          val: padNumHexSlotValue(variable, storageObj.offset),
+          val: padBytesHexSlotValue(variable, storageObj.offset),
         },
       ];
     } else if (variableType.label === 'bool') {
@@ -381,19 +381,19 @@ function encodeVariable(
       // Mapping keys are encoded depending on the key type.
       let key: string;
       if (variableType.key.startsWith('t_uint')) {
-        key = BigNumber.from(varName).toHexString();
+        key = padNumHexSlotValue(BigNumber.from(varName).toHexString(), 0);
       } else if (variableType.key.startsWith('t_bytes')) {
-        key = '0x' + remove0x(varName).padEnd(64, '0');
+        key = padBytesHexSlotValue('0x' + remove0x(varName).padEnd(64, '0'), 0);
       } else {
         // Seems to work for everything else.
-        key = varName;
+        key = padBytesHexSlotValue(varName, 0);
       }
 
       // Figure out the base slot key that the mapped values need to work off of.
       // If baseSlotKey is defined here, then we're inside of a nested mapping and we should work
       // off of that previous baseSlotKey. Otherwise the base slot will be the slot of this map.
       const prevBaseSlotKey = baseSlotKey || padNumHexSlotValue(storageObj.slot, 0);
-      const nextBaseSlotKey = ethers.utils.keccak256(padNumHexSlotValue(key, 0) + remove0x(prevBaseSlotKey));
+      const nextBaseSlotKey = ethers.utils.keccak256(key + remove0x(prevBaseSlotKey));
 
       // Encode the value. We need to use a dummy storageObj here because the function expects it.
       // Of course, we're not mapping to a specific variable. We map to a variable /type/. So we
@@ -656,12 +656,12 @@ async function getMappingTypeStorageSlots(
   // In this part we calculate the `h(k)` where k is the mapping key the user provided and h is a function that is applied to the key depending on its type
   let mappKey: string;
   if (storageObjectType.key.startsWith('t_uint')) {
-    mappKey = BigNumber.from(mappingKey[0]).toHexString();
+    mappKey = padNumHexSlotValue(BigNumber.from(mappingKey[0]).toHexString(), 0);
   } else if (storageObjectType.key.startsWith('t_bytes')) {
-    mappKey = '0x' + remove0x(mappingKey[0] as string).padEnd(64, '0');
+    mappKey = padBytesHexSlotValue('0x' + remove0x(mappingKey[0] as string).padEnd(64, '0'), 0);
   } else {
     // Seems to work for everything else.
-    mappKey = mappingKey[0] as string;
+    mappKey = padBytesHexSlotValue(mappingKey[0] as string, 0);
   }
 
   // Figure out the base slot key that the mapped values need to work off of.
@@ -669,7 +669,7 @@ async function getMappingTypeStorageSlots(
   // off of that previous baseSlotKey. Otherwise the base slot will be the key we already have.
   const prevBaseSlotKey = baseSlotKey || key;
   // Since we have `h(k) = mappKey` and `p = key` now we can calculate the slot key
-  let nextSlotKey = ethers.utils.keccak256(padNumHexSlotValue(mappKey, 0) + remove0x(prevBaseSlotKey));
+  let nextSlotKey = ethers.utils.keccak256(mappKey + remove0x(prevBaseSlotKey));
 
   let slotKeysTypes: StorageSlotKeyTypePair[] = [];
 


Test patch:
diff --git a/test/contracts/mock/StorageGetter.sol b/test/contracts/mock/StorageGetter.sol
index c458dc3..f7456f7 100644
--- a/test/contracts/mock/StorageGetter.sol
+++ b/test/contracts/mock/StorageGetter.sol
@@ -29,7 +29,7 @@ contract StorageGetter {
   PackedStruct internal _packedStruct;
   mapping(uint256 => uint256) _uint256Map;
   mapping(uint256 => mapping(uint256 => uint256)) _uint256NestedMap;
-  mapping(bytes5 => bool) _bytes5ToBoolMap;
+  mapping(bytes32 => bool) _bytes32ToBoolMap;
   mapping(address => bool) _addressToBoolMap;
   mapping(address => address) _addressToAddressMap;
   uint256[] internal _uint256Array;
@@ -119,8 +119,8 @@ contract StorageGetter {
     return _uint256NestedMap[_keyA][_keyB];
   }
 
-  function getBytes5ToBoolMapValue(bytes5 _key) public view returns (bool _out) {
-    return _bytes5ToBoolMap[_key];
+  function getBytes32ToBoolMapValue(bytes32 _key) public view returns (bool _out) {
+    return _bytes32ToBoolMap[_key];
   }
 
   function getAddressToBoolMapValue(address _key) public view returns (bool _out) {
diff --git a/test/unit/mock/editable-storage-logic.spec.ts b/test/unit/mock/editable-storage-logic.spec.ts
index 00f13f0..5e5e398 100644
--- a/test/unit/mock/editable-storage-logic.spec.ts
+++ b/test/unit/mock/editable-storage-logic.spec.ts
@@ -4,6 +4,7 @@ import { ADDRESS_EXAMPLE, BYTES32_EXAMPLE, BYTES_EXAMPLE } from '@test-utils';
 import { StorageGetter, StorageGetter__factory } from '@typechained';
 import { expect } from 'chai';
 import { BigNumber, utils } from 'ethers';
+import { defaultAbiCoder, keccak256 } from 'ethers/lib/utils';
 
 describe('Mock: Editable storage logic', () => {
   let storageGetterFactory: MockContractFactory<StorageGetter__factory>;
@@ -125,12 +126,12 @@ describe('Mock: Editable storage logic', () => {
       expect(await mock.getConstructorUint256()).to.equal(1234);
     });
 
-    it('should be able to set values in a bytes5 => bool mapping', async () => {
-      const mapKey = '0x0000005678';
+    it('should be able to set values in a bytes32 => bool mapping', async () => {
+      const mapKey = keccak256(defaultAbiCoder.encode(['bytes32'], [BYTES32_EXAMPLE]));
       const mapValue = true;
-      await mock.setVariable('_bytes5ToBoolMap', { [mapKey]: mapValue });
+      await mock.setVariable('_bytes32ToBoolMap', { [mapKey]: mapValue });
 
-      expect(await mock.getBytes5ToBoolMapValue(mapKey)).to.equal(mapValue);
+      expect(await mock.getBytes32ToBoolMapValue(mapKey)).to.equal(mapValue);
     });
 
     it('should be able to set values in a address => bool mapping', async () => {
@@ -260,7 +261,7 @@ describe('Mock: Editable storage logic', () => {
       const mapKeyB = 5678;
       const mapValue = 4321;
       const mapValueB = 8765;
-      const mapKeybytes5ToBool = '0x0000005678';
+      const mapKeybytes32ToBool = BYTES32_EXAMPLE;
       const mapValueAddress = '0x063bE0Af9711a170BE4b07028b320C90705fec7C';
       await mock.setVariables({
         _address: ADDRESS_EXAMPLE,
@@ -276,7 +277,7 @@ describe('Mock: Editable storage logic', () => {
             [mapKeyB]: mapValueB,
           },
         },
-        _bytes5ToBoolMap: { [mapKeybytes5ToBool]: true },
+        _bytes32ToBoolMap: { [mapKeybytes32ToBool]: true },
         _addressToBoolMap: { [ADDRESS_EXAMPLE]: false },
         _addressToAddressMap: { [ADDRESS_EXAMPLE]: mapValueAddress },
       });
@@ -290,7 +291,7 @@ describe('Mock: Editable storage logic', () => {
       expect(convertStructToPojo(await mock.getSimpleStruct())).to.deep.equal(struct);
       expect(await mock.getUint256MapValue(mapKey)).to.equal(mapValue);
       expect(await mock.getNestedUint256MapValue(mapKey, mapKeyB)).to.equal(mapValueB);
-      expect(await mock.getBytes5ToBoolMapValue(mapKeybytes5ToBool)).to.equal(true);
+      expect(await mock.getBytes32ToBoolMapValue(mapKeybytes32ToBool)).to.equal(true);
       expect(await mock.getAddressToBoolMapValue(ADDRESS_EXAMPLE)).to.equal(false);
       expect(await mock.getAddressToAddressMapValue(ADDRESS_EXAMPLE)).to.equal(mapValueAddress);
     });
diff --git a/test/unit/mock/readable-storage-logic.spec.ts b/test/unit/mock/readable-storage-logic.spec.ts
index df1ae45..535d827 100644
--- a/test/unit/mock/readable-storage-logic.spec.ts
+++ b/test/unit/mock/readable-storage-logic.spec.ts
@@ -121,13 +121,13 @@ describe('Mock: Readable storage logic', () => {
       expect(getValue).to.equal(await mock.getUint256MapValue(mapKey));
     });
 
-    it('should be able to get values in a bytes5 => bool mapping', async () => {
-      const mapKey = '0x0000005678';
+    it('should be able to get values in a bytes32 => bool mapping', async () => {
+      const mapKey = BYTES32_EXAMPLE;
       const mapValue = true;
-      await mock.setVariable('_bytes5ToBoolMap', { [mapKey]: mapValue });
+      await mock.setVariable('_bytes32ToBoolMap', { [mapKey]: mapValue });
 
-      const getValue = await mock.getVariable('_bytes5ToBoolMap', [mapKey]);
-      expect(getValue).to.equal(await mock.getBytes5ToBoolMapValue(mapKey));
+      const getValue = await mock.getVariable('_bytes32ToBoolMap', [mapKey]);
+      expect(getValue).to.equal(await mock.getBytes32ToBoolMapValue(mapKey));
     });
 
     it('should be able to get a nested uint256 mapping value', async () => {