Funds might be unable to collect if a driver is blocklisted.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/AddressDriver.sol#L60-L63 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/NFTDriver.sol#L109-L117 https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/DripsHub.sol#L386

Vulnerability details

Impact

Funds might be unable to collect if a driver is blocklisted.

Proof of Concept

As we can see from AddressDriver.collect(), the collected funds from DripsHub are transferred to the driver contract first and to the custom address.

File: 2023-01-drips\src\AddressDriver.sol
60:     function collect(IERC20 erc20, address transferTo) public whenNotPaused returns (uint128 amt) {
61:         amt = dripsHub.collect(callerUserId(), erc20);
62:         erc20.safeTransfer(transferTo, amt);
63:     }

File: 2023-01-drips\src\DripsHub.sol
386:     function collect(uint256 userId, IERC20 erc20)
387:         public
388:         whenNotPaused
389:         onlyDriver(userId)
390:         returns (uint128 amt)
391:     {
392:         amt = Splits._collect(userId, _assetId(erc20));
393:         _decreaseTotalBalance(erc20, amt);
394:         erc20.safeTransfer(msg.sender, amt); //@audit if driver is blocklisted?
395:     }

As we can see here, some tokens including USDC and USDT have a blocklist and the below scenario would be possible.

1. A user deployed a driver contract similar to AddressDriver or NFTDriver and he didn't add a migration option to change the driver contract. (Currently, AddressDriver and NFTDriver doesn't have the option as well.)
2. After that, he added some users and added some funds.
3. Unfortunately, the driver contract was blocklisted and he can't collect funds from all users because the funds should go through the driver contract.

Tools Used

Manual Review

Recommended Mitigation Steps

I think we don't need to transfer the funds two times, from DripsHub to Driver, from Driver to transferTo address.

We can modify the collect() functions like below to transfer the funds directly.

    //AddressDriver.sol
    function collect(IERC20 erc20, address transferTo) public whenNotPaused returns (uint128 amt) {
        amt = dripsHub.collect(callerUserId(), erc20, transferTo);
    }

    //DripsHub.sol
    function collect(uint256 userId, IERC20 erc20, address transferTo)
        public
        whenNotPaused
        onlyDriver(userId)
        returns (uint128 amt)
    {
        amt = Splits._collect(userId, _assetId(erc20));
        _decreaseTotalBalance(erc20, amt);
        erc20.safeTransfer(transferTo, amt);
    }

[GalloDaSballo]

Seems valid, I believe historically Med was awarded.

Am thinking the damage is marginal and limited only to the recipient, which would not be able to receive the tokens, so arguably the revert would be a good thing

Am thinking QA, but will definitely discuss with Judges and will flag to the Sponsor

[xmxanuel]

I get the point. However, if the entire DripsHub would be blacklisted then nothing would work anymore and we see the Drivers at least developed by us as part of Drips.

However, I agree that we could add such transferTo parameter which could save one unnecessary transferFrom``call. In general, it depends on the Driver implementation if it is needed or it if would be equal tomsg.sender`

So a valid finding.

We need to discuss the severity.

[CodeSandwich]

[disagree with severity: gas optimization] I disagree that blacklisting is an issue we should be trying to defend from. Any protocol can be taken down if its contract are blacklisted and there's nothing that can be done about it, even the proposed mitigation will work partially and in a selective blacklisting case.

I agree that the extra transferring step in some cases is unnecessary and it wastes gas. Transferring funds INTO the protocol must be done with the stop at the driver, but transferring OUT often can be done directly, without the driver in the middle. I agree that it's the driver who should decide, DripsHub should follow the driver's strategy.

[c4-sponsor]

CodeSandwich marked the issue as disagree with severity

[GalloDaSballo]

While arguably the funds would stop working, we know that the tx will revert if either target is blocked from receiving funds, meaning the tx will revert

Due to that, I believe Low Severity to be the most appropriate as no invariant is broken

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-b