Omniscia Alliance Block Audit
LibDiamond Code Style Findings
LibDiamond Code Style Findings
LDD-01C: Duplication of Code
| Type | Severity | Location |
|---|---|---|
| Gas Optimization | Informational | LibDiamond.sol:L130-L220 |
Description:
Certain notions and statements are replicated across these functions that can be better refactored into independent functions, like removeFunction.
Example:
130function addFunctions(address _facetAddress, bytes4[] memory _functionSelectors) internal {131 require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");132 DiamondStorage storage ds = diamondStorage();133 // uint16 selectorCount = uint16(diamondStorage().selectors.length);134 require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");135 uint16 selectorPosition = uint16(ds.facetFunctionSelectors[_facetAddress].functionSelectors.length);136 // add new facet address if it does not exist137 if (selectorPosition == 0) {138 enforceHasContractCode(_facetAddress, "LibDiamondCut: New facet has no code");139 ds.facetFunctionSelectors[_facetAddress].facetAddressPosition = uint16(ds.facetAddresses.length);140 ds.facetAddresses.push(_facetAddress);141 }142 for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {143 bytes4 selector = _functionSelectors[selectorIndex];144 address oldFacetAddress = ds.selectorToFacetAndPosition[selector].facetAddress;145 require(oldFacetAddress == address(0), "LibDiamondCut: Can't add function that already exists");146 ds.facetFunctionSelectors[_facetAddress].functionSelectors.push(selector);147 ds.selectorToFacetAndPosition[selector].facetAddress = _facetAddress;148 ds.selectorToFacetAndPosition[selector].functionSelectorPosition = selectorPosition;149 selectorPosition++;150 }151}152
153function replaceFunctions(address _facetAddress, bytes4[] memory _functionSelectors) internal {154 require(_functionSelectors.length > 0, "LibDiamondCut: No selectors in facet to cut");155 DiamondStorage storage ds = diamondStorage();156 require(_facetAddress != address(0), "LibDiamondCut: Add facet can't be address(0)");157 uint16 selectorPosition = uint16(ds.facetFunctionSelectors[_facetAddress].functionSelectors.length);158 // add new facet address if it does not exist159 if (selectorPosition == 0) {160 enforceHasContractCode(_facetAddress, "LibDiamondCut: New facet has no code");161 ds.facetFunctionSelectors[_facetAddress].facetAddressPosition = uint16(ds.facetAddresses.length);162 ds.facetAddresses.push(_facetAddress);163 }164 for (uint256 selectorIndex; selectorIndex < _functionSelectors.length; selectorIndex++) {165 bytes4 selector = _functionSelectors[selectorIndex];166 address oldFacetAddress = ds.selectorToFacetAndPosition[selector].facetAddress;167 require(oldFacetAddress != _facetAddress, "LibDiamondCut: Can't replace function with same function");168 removeFunction(oldFacetAddress, selector);169 // add function170 ds.selectorToFacetAndPosition[selector].functionSelectorPosition = selectorPosition;171 ds.facetFunctionSelectors[_facetAddress].functionSelectors.push(selector);172 ds.selectorToFacetAndPosition[selector].facetAddress = _facetAddress;173 selectorPosition++;174 }175}Recommendation:
We advise them to be done so to ensure code maintainability and legibility.
Alleviation:
The addition of a new function was refactored into its dedicated function thus reducing the bytecode of the contract significantly, using the Diamond storage pointer in each instance to further reduce the gas required by the function.
LDD-02C: Inefficient Data Types
| Type | Severity | Location |
|---|---|---|
| Gas Optimization | Informational | LibDiamond.sol:L17, L22 |
Description:
The EVM is made to operate on 256-bit data types and as such, operations on less than that length will cost more gas due to the unpadding and padding operations performed.
Example:
15struct FacetAddressAndPosition {16 address facetAddress;17 uint16 functionSelectorPosition; // position in facetFunctionSelectors.functionSelectors array18}19
20struct FacetFunctionSelectors {21 bytes4[] functionSelectors;22 uint16 facetAddressPosition; // position of facetAddress in facetAddresses array23}Recommendation:
We advise the linked variables to be expanded to fill the remaining space of their already reserved slots to ensure less operations are needed at the EVM level. For the first link, this means (256 - address) -> (256 - 160) -> 96 and for the latter it means a full uint256 slot as it is "packed" with a dynamic array.
Alleviation:
The development team has acknowledged this exhibit but decided to not apply its remediation in the current version of the codebase citing time constraints.
LDD-03C: Inefficient Implementation
| Type | Severity | Location |
|---|---|---|
| Gas Optimization | Informational | LibDiamond.sol:L85-L104 |
Description:
The validateSignatures function ensures that no duplicate signatures exist by iterating through the previously computed signatures, computing them again and checking whether the signers equal the current signer.
Example:
95function validateSignatures(bytes32 ethHash, bytes[] memory signatures) internal view {96 for (uint256 i = 0; i < signatures.length; i++) {97 address signer = ECDSA.recover(ethHash, signatures[i]);98 require(isMember(signer), "Router: invalid signer");99 for (uint256 j = 0; j < i; j++) {100 address signer2 = ECDSA.recover(ethHash, signatures[j]);101 require(signer != signer2, "Router: duplicate signatures");102 }103 }104}Recommendation:
We advise an in-memory array to be declared at the start of the function with the same length as the signatures array that stores address and is assigned to on each iteration of the for loop, ensuring that the nested for loop will not re-compute ECDSA.recover and will instead access the in-memory array directly reducing the gas cost of the function.
Alleviation:
The membership-controlled contract implementation was replaced by a singular LibGovernance implementation thus no longer rendering this exhibit applicable.