The setDrips() function gives the wrong accounting of the total balance of ERC20 tokens.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/9fd776b50f4be23ca038b1d0426e63a69c7a511d/src/DripsHub.sol#L510-L538

Vulnerability details

Impact

Detailed description of the impact of this finding. The setDrips() function gives the wrong accounting of the total balance of ERC20 tokens.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The setDrips() function gives the wrong accounting of the total balance of ERC20 tokens because:

1) it first updates the total balance based on the user's expected balanceDelta. No tokens were sent to the contract when the following updates occur.

 if (balanceDelta > 0) {
            _increaseTotalBalance(erc20, uint128(balanceDelta));
        }

2) After the realBalanceDelta is calculated, the previous increase is not reversed either, especially when realBalanceDelta > 0, this is important when realBalanceDelta != balanceDelta.

    if (realBalanceDelta > 0) {
            erc20.safeTransferFrom(msg.sender, address(this), uint128(realBalanceDelta));
        } else if (realBalanceDelta < 0) {
            _decreaseTotalBalance(erc20, uint128(-realBalanceDelta));
            erc20.safeTransfer(msg.sender, uint128(-realBalanceDelta));
        }

3) in summary the accounting is not correct. WE need to account based on the real ERC20 token transfers.

Tools Used

Remix

Recommended Mitigation Steps

We account for the total balance of ERC20 based on the real transfer transactions.

 function setDrips(
        uint256 userId,
        IERC20 erc20,
        DripsReceiver[] memory currReceivers,
        int128 balanceDelta,
        DripsReceiver[] memory newReceivers,
        // slither-disable-next-line similar-names
        uint32 maxEndHint1,
        uint32 maxEndHint2
    ) public whenNotPaused onlyDriver(userId) returns (int128 realBalanceDelta) {
-        if (balanceDelta > 0) {
-           _increaseTotalBalance(erc20, uint128(balanceDelta));
-       }
        realBalanceDelta = Drips._setDrips(
            userId,
            _assetId(erc20),
            currReceivers,
            balanceDelta,
            newReceivers,
            maxEndHint1,
            maxEndHint2
        );
        if (realBalanceDelta > 0) {
            erc20.safeTransferFrom(msg.sender, address(this), uint128(realBalanceDelta));
+          _increaseTotalBalance(erc20, uint128(realBalanceDelta));
        } else if (realBalanceDelta < 0) {
            _decreaseTotalBalance(erc20, uint128(-realBalanceDelta));
            erc20.safeTransfer(msg.sender, uint128(-realBalanceDelta));
        }
    }

[GalloDaSballo]

Would have benefited by a coded POC

[CodeSandwich]

[dispute validity] When balanceDelta > 0, it's never adjusted, it's the case only when balanceDelta < 0, so it's safe to _increaseTotalBalance at the beginning. It also has the benefit that when calling Drips._setDrips it's guaranteed that the total balance will never bee too high, so _setDrips can make assumptions about overflows safety.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

Disputing per Sponsor's reasoning + ERC777 / odd ERC20 are OOS per Readme

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof