Modifiers lacks check-effect-interact patters on sensitive state changing variables

[code423n4]

Lines of code

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

Vulnerability details

Impact

• Solidity functions should always use the Checks-Effects-Interactions pattern which states that the initial stage will contain only checks and validations which reside in the modifiers.

• Due to this reason, modifiers should only implement checks and validations inside of it and should not make state changes or external calls.

• The contract DripsHub was found to be violating this pattern and the modifier onlyDriver was making sensitive state changes and modifications.

Proof of Concept

Github

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

    modifier onlyDriver(uint256 userId) {
        uint32 driverId = uint32(userId >> DRIVER_ID_OFFSET);
        _assertCallerIsDriver(driverId);
        _;
    }

• Check-effect-interact patterns are missed on state changing sensitive modifiers.

Tools Used

Manual

Recommended Mitigation Steps

• Only use modifiers for implementing checks and validations. Do not make external calls or state changing actions inside modifiers.

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[GalloDaSballo]

This modifier is exactly only doing CE of CEI