Merge pull request #144 from 1inch/feature/safety-at-in-libs

[SC-1156] Safety `at` method in AddressArray and AddressSet libs
1inch · May 9, 2024 · 176d078 · 176d078
2 parents e5ff68f + 7f41d55
commit 176d078
Show file tree

Hide file tree

Showing 10 changed files with 142 additions and 14 deletions.
diff --git a/contracts/libraries/AddressArray.sol b/contracts/libraries/AddressArray.sol
@@ -46,12 +46,23 @@ library AddressArray {
     }
 
     /**
-     * @notice Retrieves the address at a specified index in the array.
+     * @notice Retrieves the address at a specified index in the array. Reverts if the index is out of bounds.
      * @param self The instance of the Data struct.
      * @param i The index to retrieve the address from.
      * @return The address stored at the specified index.
      */
     function at(Data storage self, uint256 i) internal view returns (address) {
+        if (length(self) <= i) revert IndexOutOfBounds();
+        return address(uint160(self._raw[i] & _ADDRESS_MASK));
+    }
+
+    /**
+     * @notice Retrieves the address at a specified index in the array without bounds checking.
+     * @param self The instance of the Data struct.
+     * @param i The index to retrieve the address from.
+     * @return The address stored at the specified index.
+     */
+    function unsafeAt(Data storage self, uint256 i) internal view returns (address) {
         if (i >= 1 << 32) revert IndexOutOfBounds();
         return address(uint160(self._raw[i] & _ADDRESS_MASK));
     }

diff --git a/contracts/libraries/AddressSet.sol b/contracts/libraries/AddressSet.sol
@@ -33,7 +33,7 @@ library AddressSet {
     }
 
     /**
-     * @notice Retrieves the address at a specified index in the set.
+     * @notice Retrieves the address at a specified index in the set. Reverts if the index is out of bounds.
      * @param s The set of addresses.
      * @param index The index of the address to retrieve.
      * @return The address at the specified index.
@@ -42,6 +42,16 @@ library AddressSet {
         return s.items.at(index);
     }
 
+    /**
+     * @notice Retrieves the address at a specified index in the set without bounds checking.
+     * @param s The set of addresses.
+     * @param index The index of the address to retrieve.
+     * @return The address at the specified index.
+     */
+    function unsafeAt(Data storage s, uint256 index) internal view returns (address) {
+        return s.items.unsafeAt(index);
+    }
+
     /**
      * @notice Checks if the set contains the specified address.
      * @param s The set of addresses.

diff --git a/contracts/mixins/BySig.sol b/contracts/mixins/BySig.sol
@@ -197,7 +197,7 @@ abstract contract BySig is Context, EIP712 {
         if (length == 0) {
             return super._msgSender();
         }
-        return _msgSenders.at(length - 1);
+        return _msgSenders.unsafeAt(length - 1);
     }
 
     function _useNonce(address signer, BySigTraits.Value traits, bytes calldata data) private returns(bool) {

diff --git a/contracts/tests/mocks/AddressArrayMock.sol b/contracts/tests/mocks/AddressArrayMock.sol
@@ -19,6 +19,10 @@ contract AddressArrayMock {
         return AddressArray.at(_self, i);
     }
 
+    function unsafeAt(uint256 i) external view returns (address) {
+        return AddressArray.unsafeAt(_self, i);
+    }
+
     function get() external view returns (address[] memory arr) {
         return AddressArray.get(_self);
     }

diff --git a/contracts/tests/mocks/AddressSetMock.sol b/contracts/tests/mocks/AddressSetMock.sol
@@ -17,6 +17,10 @@ contract AddressSetMock {
         return AddressSet.at(_self, index);
     }
 
+    function unsafeAt(uint256 index) external view returns (address) {
+        return AddressSet.unsafeAt(_self, index);
+    }
+
     function contains(address item) external view returns (bool) {
         return AddressSet.contains(_self, item);
     }

diff --git a/docs/contracts/libraries/AddressArray.md b/docs/contracts/libraries/AddressArray.md
@@ -11,6 +11,7 @@ _This library provides basic functionalities such as push, pop, set, and retriev
 ### Functions list
 - [length(self) internal](#length)
 - [at(self, i) internal](#at)
+- [unsafeAt(self, i) internal](#unsafeat)
 - [get(self) internal](#get)
 - [get(self, input) internal](#get)
 - [push(self, account) internal](#push)
@@ -60,7 +61,27 @@ Returns the number of addresses stored in the array.
 ```solidity
 function at(struct AddressArray.Data self, uint256 i) internal view returns (address)
 ```
-Retrieves the address at a specified index in the array.
+Retrieves the address at a specified index in the array. Reverts if the index is out of bounds.
+
+#### Parameters
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| self | struct AddressArray.Data | The instance of the Data struct. |
+| i | uint256 | The index to retrieve the address from. |
+
+#### Return Values
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+[0] | address | The address stored at the specified index. |
+
+### unsafeAt
+
+```solidity
+function unsafeAt(struct AddressArray.Data self, uint256 i) internal view returns (address)
+```
+Retrieves the address at a specified index in the array without bounds checking.
 
 #### Parameters
 

diff --git a/docs/contracts/libraries/AddressSet.md b/docs/contracts/libraries/AddressSet.md
@@ -10,6 +10,7 @@ Utilizes the AddressArray library for underlying data storage.
 ### Functions list
 - [length(s) internal](#length)
 - [at(s, index) internal](#at)
+- [unsafeAt(s, index) internal](#unsafeat)
 - [contains(s, item) internal](#contains)
 - [get(s) internal](#get)
 - [get(s, input) internal](#get)
@@ -55,7 +56,27 @@ Determines the number of addresses in the set.
 ```solidity
 function at(struct AddressSet.Data s, uint256 index) internal view returns (address)
 ```
-Retrieves the address at a specified index in the set.
+Retrieves the address at a specified index in the set. Reverts if the index is out of bounds.
+
+#### Parameters
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| s | struct AddressSet.Data | The set of addresses. |
+| index | uint256 | The index of the address to retrieve. |
+
+#### Return Values
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+[0] | address | The address at the specified index. |
+
+### unsafeAt
+
+```solidity
+function unsafeAt(struct AddressSet.Data s, uint256 index) internal view returns (address)
+```
+Retrieves the address at a specified index in the set without bounds checking.
 
 #### Parameters
 

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@1inch/solidity-utils",
-  "version": "4.2.1",
+  "version": "5.0.0",
   "main": "dist/src/index.js",
   "types": "dist/src/index.d.ts",
   "exports": {

diff --git a/test/contracts/AddressArray.test.ts b/test/contracts/AddressArray.test.ts
@@ -35,17 +35,22 @@ describe('AddressArray', function () {
     });
 
     describe('at', function () {
-        it('should get from empty array', async function () {
+        it('should not get from empty array', async function () {
+            const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
+            await expect(addressArrayMock.at(0)).to.be.revertedWithCustomError(addressArrayMock, 'IndexOutOfBounds');
+        });
+
+        it('should not get index out of array length', async function () {
             const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
-            expect(await addressArrayMock.at(0)).to.be.equal(constants.ZERO_ADDRESS);
-            expect(await addressArrayMock.at(1)).to.be.equal(constants.ZERO_ADDRESS);
+            await addressArrayMock.push(signer1);
+            await expect(addressArrayMock.at(await addressArrayMock.length())).to.be.revertedWithCustomError(addressArrayMock, 'IndexOutOfBounds');
         });
 
         it('should get from array with 1 element', async function () {
             const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
             await addressArrayMock.push(signer1);
             expect(await addressArrayMock.at(0)).to.be.equal(signer1.address);
-            expect(await addressArrayMock.at(1)).to.be.equal(constants.ZERO_ADDRESS);
+            await expect(addressArrayMock.at(1)).to.be.revertedWithCustomError(addressArrayMock, 'IndexOutOfBounds');
         });
 
         it('should get from array with several elements', async function () {
@@ -57,6 +62,29 @@ describe('AddressArray', function () {
         });
     });
 
+    describe('unsafeAt', function () {
+        it('should get from empty array', async function () {
+            const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
+            expect(await addressArrayMock.unsafeAt(0)).to.be.equal(constants.ZERO_ADDRESS);
+            expect(await addressArrayMock.unsafeAt(1)).to.be.equal(constants.ZERO_ADDRESS);
+        });
+
+        it('should get from array with 1 element', async function () {
+            const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
+            await addressArrayMock.push(signer1);
+            expect(await addressArrayMock.unsafeAt(0)).to.be.equal(signer1.address);
+            expect(await addressArrayMock.unsafeAt(1)).to.be.equal(constants.ZERO_ADDRESS);
+        });
+
+        it('should get from array with several elements', async function () {
+            const { addressArrayMock } = await loadFixture(deployAddressArrayMock);
+            await addressArrayMock.push(signer1);
+            await addressArrayMock.push(signer2);
+            expect(await addressArrayMock.unsafeAt(0)).to.be.equal(signer1.address);
+            expect(await addressArrayMock.unsafeAt(1)).to.be.equal(signer2.address);
+        });
+    });
+
     describe('get', function () {
         it('should get empty array', async function () {
             const { addressArrayMock } = await loadFixture(deployAddressArrayMock);

diff --git a/test/contracts/AddressSet.test.ts b/test/contracts/AddressSet.test.ts
@@ -33,17 +33,23 @@ describe('AddressSet', function () {
     });
 
     describe('at', function () {
-        it('should get from empty set', async function () {
+        it('should not get from empty set', async function () {
+            const { addressSetMock } = await loadFixture(deployAddressSetMock);
+            await expect(addressSetMock.at(0)).to.be.revertedWithCustomError(addressSetMock, 'IndexOutOfBounds');
+            await expect(addressSetMock.at(1)).to.be.revertedWithCustomError(addressSetMock, 'IndexOutOfBounds');
+        });
+
+        it('should not get index out of array length', async function () {
             const { addressSetMock } = await loadFixture(deployAddressSetMock);
-            expect(await addressSetMock.at(0)).to.be.equal(constants.ZERO_ADDRESS);
-            expect(await addressSetMock.at(1)).to.be.equal(constants.ZERO_ADDRESS);
+            await addressSetMock.add(signer1);
+            await expect(addressSetMock.at(await addressSetMock.length())).to.be.revertedWithCustomError(addressSetMock, 'IndexOutOfBounds');
         });
 
         it('should get from set with 1 element', async function () {
             const { addressSetMock } = await loadFixture(deployAddressSetMock);
             await addressSetMock.add(signer1);
             expect(await addressSetMock.at(0)).to.be.equal(signer1.address);
-            expect(await addressSetMock.at(1)).to.be.equal(constants.ZERO_ADDRESS);
+            await expect(addressSetMock.at(1)).to.be.revertedWithCustomError(addressSetMock, 'IndexOutOfBounds');
         });
 
         it('should get from set with several elements', async function () {
@@ -55,6 +61,29 @@ describe('AddressSet', function () {
         });
     });
 
+    describe('unsafeAt', function () {
+        it('should get from empty set', async function () {
+            const { addressSetMock } = await loadFixture(deployAddressSetMock);
+            expect(await addressSetMock.unsafeAt(0)).to.be.equal(constants.ZERO_ADDRESS);
+            expect(await addressSetMock.unsafeAt(1)).to.be.equal(constants.ZERO_ADDRESS);
+        });
+
+        it('should get from set with 1 element', async function () {
+            const { addressSetMock } = await loadFixture(deployAddressSetMock);
+            await addressSetMock.add(signer1);
+            expect(await addressSetMock.unsafeAt(0)).to.be.equal(signer1.address);
+            expect(await addressSetMock.unsafeAt(1)).to.be.equal(constants.ZERO_ADDRESS);
+        });
+
+        it('should get from set with several elements', async function () {
+            const { addressSetMock } = await loadFixture(deployAddressSetMock);
+            await addressSetMock.add(signer1);
+            await addressSetMock.add(signer2);
+            expect(await addressSetMock.unsafeAt(0)).to.be.equal(signer1.address);
+            expect(await addressSetMock.unsafeAt(1)).to.be.equal(signer2.address);
+        });
+    });
+
     describe('contains', function () {
         it('should not contain in empty set', async function () {
             const { addressSetMock } = await loadFixture(deployAddressSetMock);