The RevenueDistributor::_convertToken function will attempt to validate that the _target is authorized, however, the validation it performs is inadequate.
Specifically, all unconfigured _target values will have a fetchSelector entry of 0. This permits a malicious _target to be specified with a fallback function that will utilize all _data beyond the first 4 bytes and thus bypass the security check in RevenueDistributor::_convertToken.
Impact:
It is possible to set an arbitrary _target to swap assets, thereby permitting a complete compromise of funds held by the RevenueDistributor by a malicious distributor.
The severity of the exhibit has been reduced to medium due to requiring the distributor to act dishonestly.
Example:
/**
* @notice Converts a specific token to ETH.
* @dev This function is used to convert any token to ETH. It contains the check
* to verify that the target address and selector are correct to avoid exploits.
* @param _tokenIn Token to convert.
* @param _amount Amount to convert.
* @param _target Target address for conversion.
* @param _data Call data for conversion.
* @return _amountOut Amount ETH received.
*/
function _convertToken(
address _tokenIn,
uint256 _amount,
address _target,
bytes calldata _data
) internal returns (uint256 _amountOut) {
uint256 _before = address(this).balance;
// check if this is a pre-approved contract for swapping/converting
require(fetchSelector[_target] == bytes4(_data[0:4]), "invalid selector");
IERC20(_tokenIn).forceApprove(_target, 0);
IERC20(_tokenIn).forceApprove(_target, _amount);
(bool _success, ) = _target.call(_data);
require(_success, "low swap level call failed");
_amountOut = address(this).balance - _before;
emit RevTokenConverted(_tokenIn, _amount, _amountOut);
}
Recommendation:
We advise either a selector of 0 to be disallowed or the fetchSelector entry to have a different flag to indicate whether it has been configured, either of which we consider a valid alleviation and the latter of which would be the technically correct one as a selector of 0 is valid.
chasebrownn
commented
6 months ago
RDR-02M: Bypass of Conversion Target Authorization
Description:
The
RevenueDistributor::_convertTokenfunction will attempt to validate that the_targetis authorized, however, the validation it performs is inadequate.Specifically, all unconfigured
_targetvalues will have afetchSelectorentry of0. This permits a malicious_targetto be specified with afallbackfunction that will utilize all_databeyond the first 4 bytes and thus bypass the security check inRevenueDistributor::_convertToken.Impact:
It is possible to set an arbitrary
_targetto swap assets, thereby permitting a complete compromise of funds held by theRevenueDistributorby a malicious distributor.The severity of the exhibit has been reduced to medium due to requiring the distributor to act dishonestly.
Example:
Recommendation:
We advise either a selector of
0to be disallowed or thefetchSelectorentry to have a different flag to indicate whether it has been configured, either of which we consider a valid alleviation and the latter of which would be the technically correct one as a selector of0is valid.