code-423n4 / 2023-01-drips-findings

0 stars 2 forks source link

`driveId`s of receivers should be validated to prevent an unexpected impacts. #287

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Receiver's funds might be collected by any user or wouldn't be collected forever.

Proof of Concept

Currently, users can set any receivers in DripHub.give(), setDrips(), setSplits() functions.

File: 2023-01-drips\src\DripsHub.sol
409:     function give(uint256 userId, uint256 receiver, IERC20 erc20, uint128 amt) //@audit check driver limit
410:         public
411:         whenNotPaused
412:         onlyDriver(userId)

File: 2023-01-drips\src\DripsHub.sol
510:     function setDrips(
511:         uint256 userId,
512:         IERC20 erc20,
513:         DripsReceiver[] memory currReceivers,
514:         int128 balanceDelta,
515:         DripsReceiver[] memory newReceivers,  //@audit check driver limit
516:         // slither-disable-next-line similar-names
517:         uint32 maxEndHint1,
518:         uint32 maxEndHint2
519:     ) public whenNotPaused onlyDriver(userId) returns (int128 realBalanceDelta) {

File: 2023-01-drips\src\DripsHub.sol
576:     function setSplits(uint256 userId, SplitsReceiver[] memory receivers) //@audit check driver limit
577:         public
578:         whenNotPaused
579:         onlyDriver(userId)

But actually, there is a driver limit which is increasing one by one in registerDriver().

    function registerDriver(address driverAddr) public whenNotPaused returns (uint32 driverId) {
        DripsHubStorage storage dripsHubStorage = _dripsHubStorage();
        driverId = dripsHubStorage.nextDriverId++;
        dripsHubStorage.driverAddresses[driverId] = driverAddr;
        emit DriverRegistered(driverId, driverAddr);
    }

So if the receiver's driver id is greater than this nextDriverId, it means the receiver doesn't have his driver yet and his driver can be set randomly. So the user who registers that dirver id will collect the funds.

Also, if the driver id is too high like type(uint32).max, it's almost impossible to register such a driver id and the funds can't be collected forever.

Tools Used

Manual Review

Recommended Mitigation Steps

I suggest we should validate that the receiver's driver id is registered already for the above 3 functions.

GalloDaSballo commented 1 year ago

Would argue this would be Self-Inflicted, so am thinking QA

xmxanuel commented 1 year ago

This is technically correct. However, we see this similar to Ethereum address. I can send ETH today with Metamask to an address that might not exist.

Even if we would check if the driver exists it doesn't imply that the Driver can handle a specific subAccount of this userID.

Recommendation: Not an Issue

CodeSandwich commented 1 year ago

[dispute validity] What Manuel said.

c4-sponsor commented 1 year ago

CodeSandwich marked the issue as sponsor disputed

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

See: https://github.com/code-423n4/org/issues/53

L

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-b