Inexistent Validation of Distributor Status

[thedavidmeister]

RER-01M: Inexistent Validation of Distributor Status

Type	Severity	Location
Input Sanitization		RedeemableERC20.sol:L273-L288

Description:

The endDistribution function accepts a distributor_ argument meant to represent the actual distributor of the redeemable ERC20 from which funds should be burned / transferred from. However, no validation as to the actual status of the distributor_ exists as they are not known during construction / initialization.

Example:

function endDistribution(address distributor_)
    external
    onlyPhase(PHASE_DISTRIBUTING)
    onlyAdmin
{
    schedulePhase(PHASE_FROZEN, block.number);
    address forwardTo_ = distributionEndForwardingAddress;
    uint256 distributorBalance_ = balanceOf(distributor_);
    if (distributorBalance_ > 0) {
        if (forwardTo_ == address(0)) {
            _burn(distributor_, distributorBalance_);
        } else {
            _transfer(distributor_, forwardTo_, distributorBalance_);
        }
    }
}

Recommendation:

We strongly advise this trait of the system to be revised whereby the distributor is set during initialization as otherwise there is no guarantee the distributor passed in the function is the actual one. As an example, a "burnable" redeemable ERC20 may be defined and once the endDistribution function is called a user-address is supplied instead thereby grieving them of their funds as well as permitting the owner to retain the redeemable ERC20 in the initial distributor without necessarily setting up a forwarding address. Alternatively, a warning should be introduced that states there is no guarantee that the distributor matches the actual one and that it is up to the derivative contracts that interface with it to restrict the input address of the function.

[thedavidmeister]

The issue with this being done during initialization of the token is that for Trust the distributor is the pool address which does not exist until the auction starts (after initialization).

In develop-consolidate branch the Trust contract is deleted so this is a non-issue, the Sale can easily set the distributor during initialization.

In develop however it's not clear exactly how best to handle this. Neither Trust nor Sale are impacted by this griefing scenario as they both set the distributor themselves - Trust sets it to the pool and Sale sets it to itself.

This issue only impacts deploying the redeemable token standalone or by some third party contract that exposes the distributor to be influenced by an EOA. I'm not sure that bad workflows in a third party contract are in scope, so the issue in scope would be a standalone redeemable token deployed by an EOA. In this case it's not even clear how the intent of there being a rules-based distribution phase would even be achieved, as the EOA would be the distributor. Even if we added a "set distributor" type function to the contract, nothing would stop an EOA from calling that function immediately before ending the distribution, allowing the same kind of end-user griefing described above.