From aaf594f6dd5000ffa0e9045c2a02c44e9eff7978 Mon Sep 17 00:00:00 2001 From: Jean Cvllr <31145285+CJ42@users.noreply.github.com> Date: Tue, 15 Aug 2023 14:26:40 +0100 Subject: [PATCH] fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key (#659) * fix: add check for `length == 0` to prevent `mask` to allow any data key * test: add Foundry tests for Allowed ERC725Y Data Keys with 0 length `0x0000` in the list * test: add JS tests for `0x0000` as Allowed ERC725Y Data Keys --- .../LSP6Modules/LSP6SetDataModule.sol | 10 +- .../SetData/AllowedERC725YDataKeys.test.ts | 42 ++++ .../AllowedERC725YDataKeys.internal.ts | 94 ++++--- .../LSP6KeyManager/LSP6SetDataTest.t.sol | 230 ++++++++++++++++++ 4 files changed, 342 insertions(+), 34 deletions(-) create mode 100644 tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol index 868b0b244..bceb307b5 100644 --- a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol +++ b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol @@ -570,13 +570,14 @@ abstract contract LSP6SetDataModule { * The length of a data key is 32 bytes. * Therefore we can have a fixed allowed data key which has * a length of 32 bytes or we can have a dynamic data key - * which can have a length of up to 31 bytes. + * which can have a length from 1 up to 31 bytes. */ - if (length > 32) + if (length == 0 || length > 32) { revert InvalidEncodedAllowedERC725YDataKeys( allowedERC725YDataKeysCompacted, "couldn't DECODE from storage" ); + } /** * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & @@ -690,13 +691,14 @@ abstract contract LSP6SetDataModule { * The length of a data key is 32 bytes. * Therefore we can have a fixed allowed data key which has * a length of 32 bytes or we can have a dynamic data key - * which can have a length of up to 31 bytes. + * which can have a length from 1 up to 31 bytes. */ - if (length > 32) + if (length == 0 || length > 32) { revert InvalidEncodedAllowedERC725YDataKeys( allowedERC725YDataKeysCompacted, "couldn't DECODE from storage" ); + } /** * The bitmask discard the last `32 - length` bytes of the input data key via ANDing & diff --git a/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts b/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts index 9405de4ef..9a6b6db0b 100644 --- a/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts +++ b/tests/LSP6KeyManager/SetData/AllowedERC725YDataKeys.test.ts @@ -1712,4 +1712,46 @@ export const shouldBehaveLikeAllowedERC725YDataKeys = ( }); }); }); + + describe('`0x0000` set under AllowedERC725YDataKeys', () => { + let controllerCanSetSomeKeys: SignerWithAddress; + + const compactBytesArrayOfAllowedERC725YDataKeys = '0x0000'; + + before(async () => { + context = await buildContext(); + + controllerCanSetSomeKeys = context.accounts[1]; + + const permissionKeys = [ + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + context.owner.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:Permissions'] + + controllerCanSetSomeKeys.address.substring(2), + ERC725YDataKeys.LSP6['AddressPermissions:AllowedERC725YDataKeys'] + + controllerCanSetSomeKeys.address.substring(2), + ]; + + const permissionValues = [ + ALL_PERMISSIONS, + PERMISSIONS.SETDATA, + compactBytesArrayOfAllowedERC725YDataKeys, + ]; + + await setupKeyManager(context, permissionKeys, permissionValues); + }); + + it('should revert when trying to set `bytes32(0)` as an input data key', async () => { + const dataKey = '0x0000000000000000000000000000000000000000000000000000000000000000'; + const dataValue = '0x'; + + const setDataPayload = context.universalProfile.interface.encodeFunctionData('setData', [ + dataKey, + dataValue, + ]); + + await expect(context.keyManager.connect(controllerCanSetSomeKeys).execute(setDataPayload)) + .to.be.revertedWithCustomError(context.keyManager, 'InvalidEncodedAllowedERC725YDataKeys') + .withArgs(compactBytesArrayOfAllowedERC725YDataKeys, "couldn't DECODE from storage"); + }); + }); }; diff --git a/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts b/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts index 42fb30047..82ec9a56b 100644 --- a/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts +++ b/tests/LSP6KeyManager/internals/AllowedERC725YDataKeys.internal.ts @@ -14,6 +14,7 @@ export const testAllowedERC725YDataKeysInternals = ( ) => { describe('Testing CheckAllowedERC725YDataKeys', () => { let context: LSP6InternalsTestContext; + let dataKeys: { firstDynamicKey: DataKey; secondDynamicKey: DataKey; @@ -24,6 +25,7 @@ export const testAllowedERC725YDataKeysInternals = ( thirdFixedKey: DataKey; fourthFixedKey: DataKey; }; + let dataKeysToReturn: BytesLike[]; let compactBytesArray_2d: BytesLike; let compactBytesArray_2f: BytesLike; @@ -32,6 +34,7 @@ export const testAllowedERC725YDataKeysInternals = ( before(async () => { context = await buildContext(); + dataKeys = { firstDynamicKey: { length: '0x0011', @@ -66,6 +69,7 @@ export const testAllowedERC725YDataKeysInternals = ( key: '0x2b58178172d258515ef1d9e7c467f6f6a09510e863ef5ad383dbfc50721183df', }, }; + dataKeysToReturn = [ '0x6fae27edb0b5020ca98b9af9014331fcc79241c7f12d6afbcaea07f00e53b45d', '0xfd63b8f031e1f4c43fbb4956e08c686aa350a051d4d3e77a1e1c7f70366207b2', @@ -78,16 +82,19 @@ export const testAllowedERC725YDataKeysInternals = ( '0x9fedfe7c7366c70c52b6f11531196d0c5baa77abd16117ae30dc4eeb16dd6da2', '0x8741530fb57ca8556c5e7b45ffac62c178d8c0ff070ff1c99652e3f099997fa6', ]; + compactBytesArray_2d = dataKeys.firstDynamicKey.length + dataKeys.firstDynamicKey.key.toString().substring(2) + dataKeys.secondDynamicKey.length.toString().substring(2) + dataKeys.secondDynamicKey.key.toString().substring(2); + compactBytesArray_2f = dataKeys.firstFixedKey.length + dataKeys.firstFixedKey.key.toString().substring(2) + dataKeys.secondFixedKey.length.toString().substring(2) + dataKeys.secondFixedKey.key.toString().substring(2); + compactBytesArray_2d_2f = dataKeys.firstDynamicKey.length + dataKeys.firstDynamicKey.key.toString().substring(2) + @@ -97,6 +104,7 @@ export const testAllowedERC725YDataKeysInternals = ( dataKeys.firstFixedKey.key.toString().substring(2) + dataKeys.secondFixedKey.length.toString().substring(2) + dataKeys.secondFixedKey.key.toString().substring(2); + compactBytesArray_mixed_d_f = dataKeys.firstDynamicKey.length + dataKeys.firstDynamicKey.key.toString().substring(2) + @@ -237,8 +245,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d, @@ -257,8 +265,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d, @@ -290,8 +298,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking first fixed key: should return true', async () => { const checkedDataKey = dataKeys.firstFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2f, @@ -302,8 +310,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking second fixed key: should return true', async () => { const checkedDataKey = dataKeys.secondFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2f, @@ -343,8 +351,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d_2f, @@ -363,8 +371,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d_2f, @@ -375,8 +383,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking first fixed key: should return true', async () => { const checkedDataKey = dataKeys.firstFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d_2f, @@ -387,8 +395,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking second fixed key: should return true', async () => { const checkedDataKey = dataKeys.secondFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_2d_2f, @@ -428,8 +436,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -448,8 +456,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -468,8 +476,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -488,8 +496,8 @@ export const testAllowedERC725YDataKeysInternals = ( ) .substring(2); - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -500,8 +508,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking first fixed key: should return true', async () => { const checkedDataKey = dataKeys.firstFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -512,8 +520,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking second fixed key: should return true', async () => { const checkedDataKey = dataKeys.secondFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -524,8 +532,8 @@ export const testAllowedERC725YDataKeysInternals = ( it('checking third fixed key: should return true', async () => { const checkedDataKey = dataKeys.thirdFixedKey.key; - expect( - await context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( context.universalProfile.address, checkedDataKey, compactBytesArray_mixed_d_f, @@ -574,6 +582,32 @@ export const testAllowedERC725YDataKeysInternals = ( .withArgs(compactBytesArray_with_invalid_length, "couldn't DECODE from storage"); }); }); + + describe('checking a CompactBytesArray of AllowedERC725YDataKeys containing `0x0000`', () => { + let context: LSP6InternalsTestContext; + + before(async () => { + context = await buildContext(); + }); + + it('should revert when passing `bytes32(0)` as input data key', async () => { + const compactBytesArrayOfAllowedERC725YDataKeys = '0x0000'; + const inputDataKey = '0x0000000000000000000000000000000000000000000000000000000000000000'; + + await expect( + context.keyManagerInternalTester.verifyAllowedERC725YSingleKey( + context.universalProfile.address, + inputDataKey, + compactBytesArrayOfAllowedERC725YDataKeys, + ), + ) + .to.be.revertedWithCustomError( + context.keyManagerInternalTester, + 'InvalidEncodedAllowedERC725YDataKeys', + ) + .withArgs(compactBytesArrayOfAllowedERC725YDataKeys, "couldn't DECODE from storage"); + }); + }); }); describe('`verifyAllowedERC725YDataKeys(..)`', () => { diff --git a/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol new file mode 100644 index 000000000..75c862208 --- /dev/null +++ b/tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.4; + +import "forge-std/Test.sol"; +import "../../../contracts/UniversalProfile.sol"; +import "../../../contracts/LSP1UniversalReceiver/LSP1UniversalReceiverDelegateUP/LSP1UniversalReceiverDelegateUP.sol"; +import "../../../contracts/LSP6KeyManager/LSP6KeyManager.sol"; +import "../../../contracts/LSP14Ownable2Step/ILSP14Ownable2Step.sol"; +import "@erc725/smart-contracts/contracts/interfaces/IERC725Y.sol"; + +import {BytesLib} from "solidity-bytes-utils/contracts/BytesLib.sol"; +import { + LSP2Utils +} from "../../../contracts/LSP2ERC725YJSONSchema/LSP2Utils.sol"; +import {LSP6Utils} from "../../../contracts/LSP6KeyManager/LSP6Utils.sol"; + +import "../../../contracts/LSP1UniversalReceiver/LSP1Constants.sol"; +import "../../../contracts/LSP6KeyManager/LSP6Constants.sol"; +import "../../../contracts/LSP17ContractExtension/LSP17Constants.sol"; + +contract LSP6SetDataTest is Test { + using BytesLib for bytes; + + UniversalProfile universalProfile; + LSP1UniversalReceiverDelegateUP LSP1Delegate; + LSP6KeyManager keyManager; + + function setUp() public { + universalProfile = new UniversalProfile(address(this)); + keyManager = new LSP6KeyManager(address(universalProfile)); + LSP1Delegate = new LSP1UniversalReceiverDelegateUP(); + universalProfile.setData( + _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY, + bytes.concat(bytes20(address(LSP1Delegate))) + ); + } + + // Test for `AddressPermissions:AllowedERC725YDataKeys:` == `[0x0000] + function testFail_RevertWhenListOfAllowedERC725YDataKeyIs0x0000( + bytes32 dataKey, + bytes memory dataValue + ) public { + // dataKey cannot be LSP1, LSP6, or LSP17 data key + vm.assume(bytes16(dataKey) != _LSP6KEY_ADDRESSPERMISSIONS_ARRAY_PREFIX); + vm.assume(bytes6(dataKey) != _LSP6KEY_ADDRESSPERMISSIONS_PREFIX); + vm.assume(bytes12(dataKey) != _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX); + vm.assume(bytes12(dataKey) != _LSP17_EXTENSION_PREFIX); + + // Give owner ability to transfer ownership + bytes32 ownerDataKey = LSP2Utils.generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX, + bytes20(address(this)) + ); + universalProfile.setData( + ownerDataKey, + bytes.concat(_PERMISSION_CHANGEOWNER) + ); + + // Set permissions and allowed data keys for malicious address + address malicious = vm.addr(1234); + + bytes32 permissionsDataKey = LSP2Utils.generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX, + bytes20(malicious) + ); + bytes32 allowedERC725YDataKeysDataKey = LSP2Utils + .generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_AllowedERC725YDataKeys_PREFIX, + bytes20(malicious) + ); + + universalProfile.setData( + permissionsDataKey, + bytes.concat(_PERMISSION_SETDATA | _PERMISSION_CHANGEOWNER) + ); + + // set `0x0000` in the list of AllowedERC725YDataKeys of the controller + // this correspond to a bytes[CompactBytesArray] with only one entry + // where the length of this single `bytes` entry is `0` + universalProfile.setData( + allowedERC725YDataKeysDataKey, + bytes.concat(bytes2(0)) + ); + + // Setup KeyManager as the owner of the universalProfile + universalProfile.transferOwnership(address(keyManager)); + bytes memory payload = abi.encodeWithSelector( + ILSP14Ownable2Step.acceptOwnership.selector, + "" + ); + keyManager.execute(payload); + assert(universalProfile.owner() == address(keyManager)); + + // Verify malicious can set data for most data keys + bytes memory functionArgs = abi.encode(dataKey, dataValue); + bytes memory callData = abi.encodeWithSelector( + IERC725Y.setData.selector, + functionArgs + ); + + // CHECK it reverts when calling via the Key Manager + keyManager.execute(callData); + + // CHECK the LSP20 verification function reverts as well + keyManager.lsp20VerifyCall(malicious, 0, functionArgs); + + // CHECK it reverts when calling directly the Universal Profile + universalProfile.setData(dataKey, dataValue); + } + + // Test for + // + // ``` + // AddressPermissions:AllowedERC725YDataKeys: == [ + // allowedDataKey1, + // allowedDataKey2, + // 0x0000, + // allowedDataKey3, + // ... + // ] + function testFail_RevertWhenListOfAllowedERC725YDataKeyContains0x0000( + bytes[] memory dynamicAllowedERC725YDataKeys, + bytes32 dataKey, + bytes memory dataValue + ) public { + // dataKey cannot be LSP1, LSP6, or LSP17 data key + vm.assume(bytes16(dataKey) != _LSP6KEY_ADDRESSPERMISSIONS_ARRAY_PREFIX); + vm.assume(bytes6(dataKey) != _LSP6KEY_ADDRESSPERMISSIONS_PREFIX); + vm.assume(bytes12(dataKey) != _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX); + vm.assume(bytes12(dataKey) != _LSP17_EXTENSION_PREFIX); + + // Give owner ability to transfer ownership + bytes32 ownerDataKey = LSP2Utils.generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX, + bytes20(address(this)) + ); + universalProfile.setData( + ownerDataKey, + bytes.concat(_PERMISSION_CHANGEOWNER) + ); + + // Set permissions and allowed data keys for malicious address + address malicious = vm.addr(1234); + + bytes32 permissionsDataKey = LSP2Utils.generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX, + bytes20(malicious) + ); + bytes32 allowedERC725YDataKeysDataKey = LSP2Utils + .generateMappingWithGroupingKey( + _LSP6KEY_ADDRESSPERMISSIONS_AllowedERC725YDataKeys_PREFIX, + bytes20(malicious) + ); + + universalProfile.setData( + permissionsDataKey, + bytes.concat(_PERMISSION_SETDATA | _PERMISSION_CHANGEOWNER) + ); + + // Goal: set a list of AllowedERC725YDataKeys for the controller + // that includes one `0x0000` entry in the middle + + // 1. generate this malicious list + bytes memory maliciousCompactBytesArrayOfAllowedERC725YDataKeys = ""; + + for (uint256 ii = 0; ii < dynamicAllowedERC725YDataKeys.length; ii++) { + // We ensure for this test that each data key is dynamic up to 32 bytes + assert(dynamicAllowedERC725YDataKeys[ii].length <= 32); + + uint16 dynamicDataKeyLength = uint16( + dynamicAllowedERC725YDataKeys[ii].length + ); + + bytes memory dynamicDataKey = abi.encodePacked( + dynamicDataKeyLength, + dynamicAllowedERC725YDataKeys[ii] + ); + + // generate the CompactBytesArray of Allowed ERC725Y Data Keys + // by recursively adding the entries + + // set one 0 length entry `0x0000` in the middle, + // as shown in the comment above the function + if (ii == 2) { + maliciousCompactBytesArrayOfAllowedERC725YDataKeys = abi + .encodePacked( + maliciousCompactBytesArrayOfAllowedERC725YDataKeys, + hex"0000" + ); + } else { + maliciousCompactBytesArrayOfAllowedERC725YDataKeys = abi + .encodePacked( + maliciousCompactBytesArrayOfAllowedERC725YDataKeys, + dynamicDataKey + ); + } + } + + // 2. set this list of malicious Allowed ERC725Y Data Keys + universalProfile.setData( + allowedERC725YDataKeysDataKey, + maliciousCompactBytesArrayOfAllowedERC725YDataKeys + ); + + // Setup KeyManager as the owner of the universalProfile + universalProfile.transferOwnership(address(keyManager)); + bytes memory payload = abi.encodeWithSelector( + ILSP14Ownable2Step.acceptOwnership.selector, + "" + ); + keyManager.execute(payload); + assert(universalProfile.owner() == address(keyManager)); + + // Verify malicious can set data for most data keys + bytes memory functionArgs = abi.encode(dataKey, dataValue); + bytes memory callData = abi.encodeWithSelector( + IERC725Y.setData.selector, + functionArgs + ); + + // CHECK it reverts when calling via the Key Manager + keyManager.execute(callData); + + // CHECK the LSP20 verification function reverts as well + keyManager.lsp20VerifyCall(malicious, 0, functionArgs); + + // CHECK it reverts when calling directly the Universal Profile + universalProfile.setData(dataKey, dataValue); + } +}