TransferRewardMixerStorage Manual Review Findings

TRS-01M: Arbitrary Self-Call Construction

Type	Severity	Location
Logical Fault		TransferRewardMixerStorage.sol:L134, L135, L217, L264

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 TransferRewardMixerStorage 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 = rewardAssetTypeCodes[i];
206
207    if (!l.isRewardAssetTypeSupported[assetTypeCode]) {
208        revert UnsupportedRewardAssetType(assetTypeCode);
209    }
210
211    bytes memory callDataInput = abi.encodeWithSelector(
212        l.setterSelectorForAssetType[assetTypeCode],
213        campaignId,
214        campaignRewardData[i]
215    );
216
217    (bool success, bytes memory result) = address(this).call(callDataInput);
218    if (success == false) {
219        assembly {
220            revert(add(result, 32), mload(result))
221        }
222    }
223
224    // Store the reward asset type code for campaignId
225    campaignRewardAssetInfo.activeRewardAssetTypeCodes[i] = assetTypeCode;
226}

Recommendation:

We advise a strict subset of function signatures to be permitted as configurable in the TransferRewardMixerStorage::setCampaignTransferRewards function (i.e. Erc20RewardMinterExplicitFacet::transferErc20MinterReward, Erc20RewardTransferExplicitFacet::transferErc20Reward, Erc1155RewardMinterExplicitFacet::transferErc1155MinterReward, Erc1155RewardTransferExplicitFacet::transferErc1155Reward) 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.