fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key #659

CJ42 · 2023-08-08T07:46:08Z

What does this PR introduce?

🐛 Bug Fixes

Overview

There is an insufficient length check when validating the allowed data keys that can be set by a caller. In particular, if a decoded length in the compact bytes array happens to be 0, the caller will be validated against any data key.

Although a decoded length is not possible when setting allowed data keys via the LSP6SetDataModule contract, there is no guarantee that data values before ownership transfer to an LSP6 contract are valid. This allows a scamming opportunity as malicious actors who set-up an ERC725 contract for another can create a backdoor that cannot be easily seen as the allowed data keys will simply have an extra 0x0000.

When verifying allowed data keys that a caller can set, a length check is done on the first 2 bytes of a compact bytes array to ensure that the length does not exceed a data key's length.

            if (length > 32)
                revert InvalidEncodedAllowedERC725YDataKeys(
                    allowedERC725YDataKeysCompacted,
                    "couldn't DECODE from storage"
                );

This length value is used to decide the bit mask when validating data keys.

            mask =
                bytes32(
                    0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
                ) <<
                (8 * (32 - length));

In the case that length == 0, then mask == bytes32(0).

Then the validation check for all data keys will pass as the validation will be reduced to whether or not allowedKey & bytes32(0) == inputDataKey & bytes32(0), which is always true.

                allowedKey := and(memoryAt, mask)
            }

            if (allowedKey == (inputDataKey & mask)) return;

This allows the caller to set data values for all data keys except those used for LSP1, LSP6, and LSP17 due to the initial checks in LSP6SetDataModule._getPermissionRequiredToSetDataKey(...). In particular, they would be able to obstruct data values for the following established keys:

LSP3 keys detailing information about a universal profile
LSP5 keys detailing information about received assets
LSP10 keys detailing information about vault ownership

Implemented Fixes

add a check in LSP6SetDataModule to revert if in the extracted from the AllowedERC725YDataKeys read from the linked account storage, there is an entry in the CompactByteArray that is 0x0000, meaning the length for the entry (= the allowed data key) is 0.
Add Foundry tests to test that the scenario described above reverts

PR Checklist

Wrote Tests
Wrote & Generated Documentation (readme/natspec/dodoc)
Ran npm run lint && npm run lint:solidity (solhint)
Ran npm run format (prettier)
Ran npm run build
Ran npm run test

Hugoo · 2023-08-08T07:46:11Z

Task linked: DEV-7697 (C4 #97) - Insufficient Length Check When Verifying Allowed Data Keys

github-actions · 2023-08-08T07:48:31Z

👋 Hello
⛽ I am the Gas Bot Reporter. I keep track of the gas costs of common interactions using Universal Profiles 🆙 !
📊 Here is a summary of the gas cost with the code introduced by this PR.

⛽📊 See Gas Benchmark report of Using UniversalProfile owned by an EOA

🔀 `execute` scenarios

`execute` scenarios - 👑 UP Owner	⛽ Gas Usage
Transfer 1 LYX to an EOA without data	37537
Transfer 1 LYX to a UP without data	36639
Transfer 1 LYX to an EOA with 256 bytes of data	42210
Transfer 1 LYX to a UP with 256 bytes of data	44750
Transfer 0.1 LYX to 3x EOA without data	70862
Transfer 0.1 LYX to 3x UP without data	75680
Transfer 0.1 LYX to 3x EOA with 256 bytes of data	84850
Transfer 0.1 LYX to 3x EOA with 256 bytes of data	99898

🗄️ `setData` scenarios

`setData` scenarios - 👑 UP Owner	⛽ Gas Usage
Set a 20 bytes long value	49971
Set a 60 bytes long value	95281
Set a 160 bytes long value	164453
Set a 300 bytes long value	279700
Set a 600 bytes long value	484112
Change the value of a data key already set	32859
Remove the value of a data key already set	27333
Set 2 data keys of 20 bytes long value	78428
Set 2 data keys of 100 bytes long value	260580
Set 3 data keys of 20 bytes long value	105145
Change the value of three data keys already set of 20 bytes long value	45445
Remove the value of three data keys already set	41339

🗄️ `Tokens` scenarios

`Tokens` scenarios - 👑 UP Owner	⛽ Gas Usage
Minting a LSP7Token to a UP (No Delegate) from an EOA	91241
Minting a LSP7Token to an EOA from an EOA	59206
Transferring an LSP7Token from a UP to another UP (No Delegate)	98689
Minting a LSP8Token to a UP (No Delegate) from an EOA	158192
Minting a LSP8Token to an EOA from an EOA	126157
Transferring an LSP8Token from a UP to another UP (No Delegate)	147236

📝 Notes

The execute and setData scenarios are executed on a fresh UniversalProfile smart contract, deployed as standard contracts (not as proxy behind a base contract implementation).

⛽📊 See Gas Benchmark report of Using UniversalProfile owned by an LSP6KeyManager

This document contains the gas usage for common interactions and scenarios when using UniversalProfile smart contracts.

🔀 `execute` scenarios

👑 unrestricted controller

`execute` scenarios - 👑 main controller	⛽ Gas Usage
transfer LYX to an EOA	60439
transfer LYX to a UP	62041
transfer tokens (LSP7) to an EOA (no data)	107162
transfer tokens (LSP7) to a UP (no data)	242734
transfer a NFT (LSP8) to a EOA (no data)	171009
transfer a NFT (LSP8) to a UP (no data)	289909

🛃 restricted controller

`execute` scenarios - 🛃 restricted controller	⛽ Gas Usage
transfer some LYXes to an EOA - restricted to 1 x allowed address only (TRANSFERVALUE + 1x AllowedCalls)	72648
transfers some tokens (LSP7) to an EOA - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data)	122941
transfers some tokens (LSP7) to an other UP - restricted to LSP7 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data)	258513
transfers a NFT (LSP8) to an EOA - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data)	186776
transfers a NFT (LSP8) to an other UP - restricted to LSP8 + 2x allowed contracts only (CALL + 2x AllowedCalls) (no data)	305676

🗄️ `setData` scenarios

👑 unrestricted controller

`setData` scenarios - 👑 main controller	⛽ Gas Usage
updates profile details (LSP3Profile metadata)	136875
give permissions to a controller (AddressPermissions[] + AddressPermissions[index] + AddressPermissions:Permissions:)	132906
restrict a controller to some specific ERC725Y Data Keys	139282
restrict a controller to interact only with 3x specific addresses	161986
remove a controller (its permissions + its address from the AddressPermissions[] array)	67871
write 5x LSP12 Issued Assets	233253

🛃 restricted controller

`setData` scenarios - 🛃 restricted controller	⛽ Gas Usage
`setData(bytes32,bytes)` -> updates 1x data key	102776
`setData(bytes32[],bytes[])` -> updates 3x data keys (first x3)	161515
`setData(bytes32[],bytes[])` -> updates 3x data keys (middle x3)	145669
`setData(bytes32[],bytes[])` -> updates 3x data keys (last x3)	171002
`setData(bytes32[],bytes[])` -> updates 2x data keys + add 3x new controllers (including setting the array length + indexes under AddressPermissions[index])	249922

📝 Notes

The execute and setData scenarios are executed on a fresh UniversalProfile and LSP6KeyManager smart contracts, deployed as standard contracts (not as proxy behind a base contract implementation).

github-actions · 2023-08-08T07:50:18Z

Changes to gas cost

Generated at commit: e58995744641c515383a6a826500a42fdbc13053, compared to commit: 9db488aad9065f88cfc3c5f43f47b1fab35fc5f0

🧾 Summary (10% most significant diffs)

Contract	Method	Avg (+/-)	%
LSP6SetDataRestrictedController	execute	+13 ❌	+0.05%
LSP6SetDataUnrestrictedController	execute	+13 ❌	+0.05%

Full diff report 👇

Contract	Deployment Cost (+/-)	Method	Min (+/-)	%	Avg (+/-)	%	Median (+/-)	%	Max (+/-)	%	# Calls (+/-)
LSP6SetDataRestrictedController	2,877,091 (+38,246)	execute	18,354 (0)	0.00%	28,466 (+13)	+0.05%	28,466 (+13)	+0.05%	38,578 (+25)	+0.06%	2 (0)
LSP6SetDataUnrestrictedController	2,877,091 (+38,246)	execute	18,354 (0)	0.00%	28,466 (+13)	+0.05%	28,466 (+13)	+0.05%	38,578 (+25)	+0.06%	2 (0)
LSP6ExecuteRestrictedController	2,892,307 (+38,247)
LSP6ExecuteUnrestrictedController	2,892,307 (+38,247)

YamenMerhi · 2023-08-14T07:51:59Z

tests/foundry/LSP6KeyManager/LSP6SetDataTest.t.sol

+            functionArgs
+        );
+
+        // CHECK it reverts when calling via the Key Manager


How you are testing for revert ? I don't see any vm.expectRevert

because it using testFail_ pattern. it is expecting a revert

Ah nice, didn't see it

skimaharvey

LGTM. Interesting though that with vm.expectRevert it is not working

…0x0000` in the list

CJ42 force-pushed the DEV-7697_C4-97---Insufficient-Length-Check-When-Verifying-Allowed-Data-Keys_Jean branch 2 times, most recently from da084b1 to 79f2012 Compare August 9, 2023 10:34

CJ42 marked this pull request as ready for review August 9, 2023 10:40

CJ42 changed the title ~~fix: add check for length == 0 to prevent mask to allow any data key~~ fix: (C4 #97) add check for length == 0 to prevent mask to allow any data key Aug 9, 2023

YamenMerhi reviewed Aug 14, 2023

View reviewed changes

CJ42 force-pushed the DEV-7697_C4-97---Insufficient-Length-Check-When-Verifying-Allowed-Data-Keys_Jean branch from d43c2d8 to 0179a70 Compare August 15, 2023 12:31

skimaharvey approved these changes Aug 15, 2023

View reviewed changes

b00ste approved these changes Aug 15, 2023

View reviewed changes

CJ42 added 3 commits August 15, 2023 14:14

fix: add check for length == 0 to prevent mask to allow any data key

6d4ea60

test: add Foundry tests for Allowed ERC725Y Data Keys with 0 length `…

920f631

…0x0000` in the list

test: add JS tests for 0x0000 as Allowed ERC725Y Data Keys

456f418

CJ42 force-pushed the DEV-7697_C4-97---Insufficient-Length-Check-When-Verifying-Allowed-Data-Keys_Jean branch from 0179a70 to 456f418 Compare August 15, 2023 13:14

CJ42 merged commit aaf594f into develop Aug 15, 2023
26 checks passed

CJ42 deleted the DEV-7697_C4-97---Insufficient-Length-Check-When-Verifying-Allowed-Data-Keys_Jean branch August 15, 2023 13:26

This was referenced Nov 30, 2023

chore(main): release lsp-smart-contracts 0.13.0 #816

Merged

chore(main): release lsp-smart-contracts 0.13.0 #819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key #659

fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key #659

CJ42 commented Aug 8, 2023 •

edited

Loading

Hugoo commented Aug 8, 2023

github-actions bot commented Aug 8, 2023

🔀 `execute` scenarios

🗄️ `setData` scenarios

🗄️ `Tokens` scenarios

📝 Notes

🔀 `execute` scenarios

👑 unrestricted controller

🛃 restricted controller

🗄️ `setData` scenarios

👑 unrestricted controller

🛃 restricted controller

📝 Notes

github-actions bot commented Aug 8, 2023 •

edited

Loading

YamenMerhi Aug 14, 2023

skimaharvey Aug 14, 2023

YamenMerhi Aug 14, 2023

skimaharvey left a comment

fix: (C4 #97) add check for length == 0 to prevent mask to allow any data key #659

fix: (C4 #97) add check for length == 0 to prevent mask to allow any data key #659

Conversation

CJ42 commented Aug 8, 2023 • edited Loading

What does this PR introduce?

🐛 Bug Fixes

Overview

Implemented Fixes

PR Checklist

Hugoo commented Aug 8, 2023

github-actions bot commented Aug 8, 2023

🔀 execute scenarios

🗄️ setData scenarios

🗄️ Tokens scenarios

📝 Notes

🔀 execute scenarios

👑 unrestricted controller

🛃 restricted controller

🗄️ setData scenarios

👑 unrestricted controller

🛃 restricted controller

📝 Notes

github-actions bot commented Aug 8, 2023 • edited Loading

Changes to gas cost

🧾 Summary (10% most significant diffs)

YamenMerhi Aug 14, 2023

Choose a reason for hiding this comment

skimaharvey Aug 14, 2023

Choose a reason for hiding this comment

YamenMerhi Aug 14, 2023

Choose a reason for hiding this comment

skimaharvey left a comment

Choose a reason for hiding this comment

fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key #659

fix: (C4 #97) add check for `length == 0` to prevent `mask` to allow any data key #659

CJ42 commented Aug 8, 2023 •

edited

Loading

🔀 `execute` scenarios

🗄️ `setData` scenarios

🗄️ `Tokens` scenarios

🔀 `execute` scenarios

🗄️ `setData` scenarios

github-actions bot commented Aug 8, 2023 •

edited

Loading