refactor: disallow non alphanumeric symbols (#945)

* refactor: disallow non alphanumeric symbols * feat: allow empty spaces in "isAlphanumeric" refactor: alphabetical ordering refactor: return different string test: thorough concrete tests for "isAlphanumeric" * refactor: simplify logical conditions test: fuzz tests for "isAlphanumeric" test: shared test contract for NFT descriptor --------- Co-authored-by: Paul Razvan Berg <[email protected]>
sablier-labs · Jul 3, 2024 · 902c2d3 · 902c2d3
1 parent 0a80c28
commit 902c2d3
Show file tree

Hide file tree

Showing 13 changed files with 221 additions and 17 deletions.
diff --git a/foundry.toml b/foundry.toml
@@ -3,9 +3,9 @@
   bytecode_hash = "none"
   evm_version = "shanghai"
   fs_permissions = [
-    { access = "read", path = "./out-optimized"},
-    { access = "read", path = "package.json"},
-    { access = "read-write", path = "./benchmark/results"}
+    { access = "read", path = "./out-optimized" },
+    { access = "read", path = "package.json" },
+    { access = "read-write", path = "./benchmark/results" },
   ]
   gas_reports = [
     "SablierV2LockupDynamic",

diff --git a/precompiles/Precompiles.sol b/precompiles/Precompiles.sol
diff --git a/src/SablierV2NFTDescriptor.sol b/src/SablierV2NFTDescriptor.sol
@@ -302,6 +302,28 @@ contract SablierV2NFTDescriptor is ISablierV2NFTDescriptor {
         return string.concat("Sablier V2 ", sablierModel, " #", streamId);
     }
 
+    /// @notice Checks whether the provided string contains only alphanumeric characters and spaces.
+    /// @dev Note that this returns true for empty strings, but it is not a security concern.
+    function isAlphanumeric(string memory str) internal pure returns (bool) {
+        // Convert the string to bytes to iterate over its characters.
+        bytes memory b = bytes(str);
+
+        uint256 length = b.length;
+        for (uint256 i = 0; i < length; ++i) {
+            bytes1 char = b[i];
+
+            // Check if it's a space or an alphanumeric character.
+            bool isSpace = char == 0x20; // space
+            bool isDigit = char >= 0x30 && char <= 0x39; // 0-9
+            bool isUppercaseLetter = char >= 0x41 && char <= 0x5A; // A-Z
+            bool isLowercaseLetter = char >= 0x61 && char <= 0x7A; // a-z
+            if (!(isSpace || isDigit || isUppercaseLetter || isLowercaseLetter)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     /// @notice Maps ERC-721 symbols to human-readable model names.
     /// @dev Reverts if the symbol is unknown.
     function mapSymbol(IERC721Metadata sablier) internal view returns (string memory) {
@@ -341,11 +363,14 @@ contract SablierV2NFTDescriptor is ISablierV2NFTDescriptor {
 
         string memory symbol = abi.decode(returnData, (string));
 
-        // The length check is a precautionary measure to help mitigate potential security threats from malicious assets
-        // injecting scripts in the symbol string.
+        // Check if the symbol is too long or contains non-alphanumeric characters, this measure helps mitigate
+        // potential security threats from malicious assets injecting scripts in the symbol string.
         if (bytes(symbol).length > 30) {
             return "Long Symbol";
         } else {
+            if (!isAlphanumeric(symbol)) {
+                return "Non-Alphanumeric Symbol";
+            }
             return symbol;
         }
     }

diff --git a/test/integration/concrete/nft-descriptor/generateAccentColor.t.sol b/test/integration/concrete/nft-descriptor/generateAccentColor.t.sol
@@ -1,9 +1,9 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity >=0.8.22 <0.9.0;
 
-import { NFTDescriptor_Integration_Concrete_Test } from "./NFTDescriptor.t.sol";
+import { NFTDescriptor_Integration_Shared_Test } from "../../shared/nft-descriptor/NFTDescriptor.t.sol";
 
-contract GenerateAccentColor_Integration_Concrete_Test is NFTDescriptor_Integration_Concrete_Test {
+contract GenerateAccentColor_Integration_Concrete_Test is NFTDescriptor_Integration_Shared_Test {
     function test_GenerateAccentColor() external view {
         // Passing a dummy contract instead of a real Sablier contract to make this test easy to maintain.
         string memory actualColor = nftDescriptorMock.generateAccentColor_({ sablier: address(noop), streamId: 1337 });

diff --git a/test/integration/concrete/nft-descriptor/is-alphanumeric/isAlphanumeric.t.sol b/test/integration/concrete/nft-descriptor/is-alphanumeric/isAlphanumeric.t.sol
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.22 <0.9.0;
+
+import { NFTDescriptor_Integration_Shared_Test } from "../../../shared/nft-descriptor/NFTDescriptor.t.sol";
+
+contract IsAlphanumeric_Integration_Concrete_Test is NFTDescriptor_Integration_Shared_Test {
+    function test_IsAlphanumeric_EmptyString() external view {
+        string memory symbol = "";
+        bool result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+    }
+
+    modifier whenNotEmptyString() {
+        _;
+    }
+
+    function test_IsAlphanumeric_ContainsNonAlphanumericCharacters() external view whenNotEmptyString {
+        string memory symbol = "<foo/>";
+        bool result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo/";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo\\";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo%";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo&";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo(";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo)";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo\"";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo'";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo`";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo;";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+
+        symbol = "foo%20"; // URL-encoded empty space
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertFalse(result, "isAlphanumeric");
+    }
+
+    modifier whenOnlyAlphanumericCharacters() {
+        _;
+    }
+
+    function test_IsAlphanumeric() external view whenNotEmptyString whenOnlyAlphanumericCharacters {
+        string memory symbol = "foo";
+        bool result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "Foo";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "Foo ";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "Foo Bar";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "  ";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "foo01234";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+
+        symbol = "123456789";
+        result = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertTrue(result, "isAlphanumeric");
+    }
+}
diff --git a/test/integration/concrete/nft-descriptor/is-alphanumeric/isAlphanumeric.tree b/test/integration/concrete/nft-descriptor/is-alphanumeric/isAlphanumeric.tree
@@ -0,0 +1,8 @@
+isAlphanumeric.t.sol
+├── when the symbol is an empty string
+│  └── it should return true
+└── when the symbol is not an empty string
+   ├── given the symbol does not contain only alphanumeric characters
+   │  └── it should return false
+   └── given the symbol contains only alphanumeric characters
+      └── it should return true
diff --git a/test/integration/concrete/nft-descriptor/map-symbol/mapSymbol.t.sol b/test/integration/concrete/nft-descriptor/map-symbol/mapSymbol.t.sol
@@ -6,9 +6,9 @@ import { MockERC721 } from "forge-std/src/mocks/MockERC721.sol";
 
 import { Errors } from "src/libraries/Errors.sol";
 
-import { NFTDescriptor_Integration_Concrete_Test } from "../NFTDescriptor.t.sol";
+import { NFTDescriptor_Integration_Shared_Test } from "../../../shared/nft-descriptor/NFTDescriptor.t.sol";
 
-contract MapSymbol_Integration_Concrete_Test is NFTDescriptor_Integration_Concrete_Test {
+contract MapSymbol_Integration_Concrete_Test is NFTDescriptor_Integration_Shared_Test {
     function test_RevertGiven_UnknownNFT() external {
         MockERC721 nft = new MockERC721();
         nft.initialize("Foo", "FOO");

diff --git a/test/integration/concrete/nft-descriptor/safe-asset-decimals/safeAssetDecimals.t.sol b/test/integration/concrete/nft-descriptor/safe-asset-decimals/safeAssetDecimals.t.sol
@@ -1,9 +1,9 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity >=0.8.22 <0.9.0;
 
-import { NFTDescriptor_Integration_Concrete_Test } from "../NFTDescriptor.t.sol";
+import { NFTDescriptor_Integration_Shared_Test } from "../../../shared/nft-descriptor/NFTDescriptor.t.sol";
 
-contract SafeAssetDecimals_Integration_Concrete_Test is NFTDescriptor_Integration_Concrete_Test {
+contract SafeAssetDecimals_Integration_Concrete_Test is NFTDescriptor_Integration_Shared_Test {
     function test_SafeAssetDecimals_EOA() external view {
         address eoa = vm.addr({ privateKey: 1 });
         uint8 actualDecimals = nftDescriptorMock.safeAssetDecimals_(address(eoa));

diff --git a/test/integration/concrete/nft-descriptor/safe-asset-symbol/safeAssetSymbol.t.sol b/test/integration/concrete/nft-descriptor/safe-asset-symbol/safeAssetSymbol.t.sol
@@ -3,9 +3,9 @@ pragma solidity >=0.8.22 <0.9.0;
 
 import { ERC20Mock } from "../../../../mocks/erc20/ERC20Mock.sol";
 import { ERC20Bytes32 } from "../../../../mocks/erc20/ERC20Bytes32.sol";
-import { NFTDescriptor_Integration_Concrete_Test } from "../NFTDescriptor.t.sol";
+import { NFTDescriptor_Integration_Shared_Test } from "../../../shared/nft-descriptor/NFTDescriptor.t.sol";
 
-contract SafeAssetSymbol_Integration_Concrete_Test is NFTDescriptor_Integration_Concrete_Test {
+contract SafeAssetSymbol_Integration_Concrete_Test is NFTDescriptor_Integration_Shared_Test {
     function test_SafeAssetSymbol_EOA() external view {
         address eoa = vm.addr({ privateKey: 1 });
         string memory actualSymbol = nftDescriptorMock.safeAssetSymbol_(address(eoa));
@@ -48,7 +48,25 @@ contract SafeAssetSymbol_Integration_Concrete_Test is NFTDescriptor_Integration_
         _;
     }
 
-    function test_SafeAssetSymbol() external view whenERC20Contract givenSymbolString givenSymbolNotLong {
+    function test_SafeAssetSymbol_NonAlphanumeric() external whenERC20Contract givenSymbolString givenSymbolNotLong {
+        ERC20Mock asset = new ERC20Mock({ name: "Token", symbol: "<svg/onload=alert(\"xss\")>" });
+        string memory actualSymbol = nftDescriptorMock.safeAssetSymbol_(address(asset));
+        string memory expectedSymbol = "Non-Alphanumeric Symbol";
+        assertEq(actualSymbol, expectedSymbol, "symbol");
+    }
+
+    modifier givenSymbolAlphanumeric() {
+        _;
+    }
+
+    function test_SafeAssetSymbol()
+        external
+        view
+        whenERC20Contract
+        givenSymbolString
+        givenSymbolNotLong
+        givenSymbolAlphanumeric
+    {
         string memory actualSymbol = nftDescriptorMock.safeAssetSymbol_(address(dai));
         string memory expectedSymbol = dai.symbol();
         assertEq(actualSymbol, expectedSymbol, "symbol");

diff --git a/test/integration/concrete/nft-descriptor/safe-asset-symbol/safeAssetSymbol.tree b/test/integration/concrete/nft-descriptor/safe-asset-symbol/safeAssetSymbol.tree
@@ -8,4 +8,7 @@ safeAssetSymbol.t.sol
       ├── given the symbol is longer than 30 characters
       │  └── it should return a hard-coded values
       └── given the symbol is not longer than 30 characters
-         └── it should return the correct symbol value
+         ├── given the symbol contains non-alphanumeric characters
+         │  └── it should return a hard-coded value
+         └── given the symbol contains only alphanumeric characters
+            └── it should return the correct symbol value
diff --git a/test/integration/fuzz/nft-descriptor/isAlphanumeric.t.sol b/test/integration/fuzz/nft-descriptor/isAlphanumeric.t.sol
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity >=0.8.22 <0.9.0;
+
+import { NFTDescriptor_Integration_Shared_Test } from "../../shared/nft-descriptor/NFTDescriptor.t.sol";
+
+contract IsAlphanumeric_Integration_Fuzz_Test is NFTDescriptor_Integration_Shared_Test {
+    bytes1 internal constant SPACE = 0x20; // ASCII 32
+    bytes1 internal constant ZERO = 0x30; // ASCII 48
+    bytes1 internal constant NINE = 0x39; // ASCII 57
+    bytes1 internal constant A = 0x41; // ASCII 65
+    bytes1 internal constant Z = 0x5A; // ASCII 90
+    bytes1 internal constant a = 0x61; // ASCII 97
+    bytes1 internal constant z = 0x7A; // ASCII 122
+
+    modifier whenNotEmptyString() {
+        _;
+    }
+
+    /// @dev Given enough fuzz runs, all the following scenarios will be fuzzed:
+    ///
+    /// - String with only alphanumerical characters
+    /// - String with only non-alphanumerical characters
+    /// - String with both alphanumerical and non-alphanumerical characters
+    function testFuzz_IsAlphanumeric(string memory symbol) external view whenNotEmptyString {
+        bytes memory b = bytes(symbol);
+        uint256 length = b.length;
+        bool expectedResult = true;
+        for (uint256 i = 0; i < length; ++i) {
+            bytes1 char = b[i];
+            if (!isAlphanumericChar(char)) {
+                expectedResult = false;
+                break;
+            }
+        }
+        bool actualResult = nftDescriptorMock.isAlphanumeric_(symbol);
+        assertEq(actualResult, expectedResult, "isAlphanumeric");
+    }
+
+    function isAlphanumericChar(bytes1 char) internal pure returns (bool) {
+        bool isSpace = char == SPACE;
+        bool isDigit = char >= ZERO && char <= NINE;
+        bool isUppercaseLetter = char >= A && char <= Z;
+        bool isLowercaseLetter = char >= a && char <= z;
+        return isSpace || isDigit || isUppercaseLetter || isLowercaseLetter;
+    }
+}
diff --git a/...ncrete/nft-descriptor/NFTDescriptor.t.sol → ...shared/nft-descriptor/NFTDescriptor.t.sol b/...ncrete/nft-descriptor/NFTDescriptor.t.sol → ...shared/nft-descriptor/NFTDescriptor.t.sol
@@ -4,7 +4,7 @@ pragma solidity >=0.8.22 <0.9.0;
 import { Integration_Test } from "../../Integration.t.sol";
 import { NFTDescriptorMock } from "../../../mocks/NFTDescriptorMock.sol";
 
-abstract contract NFTDescriptor_Integration_Concrete_Test is Integration_Test {
+abstract contract NFTDescriptor_Integration_Shared_Test is Integration_Test {
     NFTDescriptorMock internal nftDescriptorMock;
 
     function setUp() public virtual override {

diff --git a/test/mocks/NFTDescriptorMock.sol b/test/mocks/NFTDescriptorMock.sol
@@ -78,6 +78,10 @@ contract NFTDescriptorMock is SablierV2NFTDescriptor {
         return SVGElements.hourglass(status);
     }
 
+    function isAlphanumeric_(string memory symbol) external pure returns (bool) {
+        return isAlphanumeric(symbol);
+    }
+
     function mapSymbol_(IERC721Metadata nft) external view returns (string memory) {
         return mapSymbol(nft);
     }