TransferInputMixerStorage Manual Review Findings

TIS-01M: Arbitrary Self-Call Construction

Type	Severity	Location
Logical Fault		TransferInputMixerStorage.sol:L134, L135, L212, L216, L256, L263

Description:

The Evergon Labs Diamond responsible for handling the various functionalities of the staking system relies on several assumptions in its internal cross-facet calls that are generally upheld via usage of the DelegateCallee::onlyInternalDelegateCall modifier, ensuring that the caller of a function is the Diamond itself.

This security measure can be bypassed entirely due to the TransferInputMixerStorage implementation that allows any selector to be set as a "setter" for a particular asset. While the campaignId being interfixed between the function selector utilized and the remaining function payload restricts the malicious actions to a particular campaign ID, it still allows certain functionality of a campaign to be accessed outside their normal execution flows.

Impact:

Although restrained in severity due to the injection of the campaignId variable, we still believe that the current arbitrary-selector input mechanism is insecure and might cause cross-facet complications especially as new facets are introduced to the Diamond implementation.

Example:

Copy
204for (uint256 i; i < length; i++) {
205    uint256 assetTypeCode = inputAssetTypeCodes[i];
206
207    if (!l.isInputAssetTypeSupported[assetTypeCode]) {
208        revert UnsupportedInputAssetType(assetTypeCode);
209    }
210
211    bytes memory callDataInput = abi.encodeWithSelector(
212        l.setterSelectorForAssetType[assetTypeCode],
213        campaignId,
214        campaignInputData[i]
215    );
216    (bool success, bytes memory result) = address(this).call(callDataInput);
217    if (success == false) {
218        assembly {
219            revert(add(result, 32), mload(result))
220        }
221    }
222
223    // Store the input asset type code for campaignId
224    campaignInputAssetInfo.activeInputAssetTypeCodes[i] = assetTypeCode;
225}

Recommendation:

We advise a strict subset of function signatures to be permitted as configurable in the TransferInputMixerStorage::setCampaignTransferInput function (i.e. Erc20InputExplicitFacet::transferErc20Input, Erc1155InputExplicitFacet::transferErc1155Input) ensuring that such malfunctions are not permitted via carefully crafted input packet configurations.

Alleviation (b64b659786cf3c84bea52feb3a69f546ba3601f0):

The function selectors for performing transfers as well as implementing setters are now strictly validated as explicitly supported, preventing arbitrary self-calls from being constructed and thus alleviating this exhibit in full.