Reentrancy due to lack of check-effect-interact pattern by valid Driver users

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L386 https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L409

Vulnerability details

Impact

• In a Re-entrancy attack, a malicious contract calls back into the calling contract before the first invocation of the function is finished. This may cause the different invocations of the function to interact in undesirable ways, especially in cases where the function is updating state variables after the external calls.

• This may lead to loss of funds, improper value updates, token loss, etc.

• Check-effect-interact patterns are missed.

Proof of Concept

Github :

• https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L386
• https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L409

    function collect(uint256 userId, IERC20 erc20)
        public
        whenNotPaused
        onlyDriver(userId)
        returns (uint128 amt)
    {
        amt = Splits._collect(userId, _assetId(erc20));
        _decreaseTotalBalance(erc20, amt);
        erc20.safeTransfer(msg.sender, amt);
    }

• The collect/give functions doesn't have a require condition which validates the user inputs, and updates the state after which transfers the amount. currently it gets transferred directly which violates the secure design principles as well as gets susceptible for reentancy bugs.

Tools Used

Manual

Recommended Mitigation Steps

• It is recommended to add a [Re-entrancy Guard] to the functions making external calls. The functions should use a Checks-Effects-Interactions pattern. The external calls should be executed at the end of the function, and all the state-changing must happen before the call.

[GalloDaSballo]

Missing Proof

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof