DripsHub.updateDriverAddress will not be called by *Driver

[code423n4]

Lines of code

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

Vulnerability details

Impact

DripsHub.updateDriverAddress is used to update the Driver address corresponding to the driverId

    function updateDriverAddress(uint32 driverId, address newDriverAddr) public whenNotPaused {
        _assertCallerIsDriver(driverId);
        _dripsHubStorage().driverAddresses[driverId] = newDriverAddr;
        emit DriverAddressUpdated(driverId, msg.sender, newDriverAddr);
    }

where _assertCallerIsDriver requires that the caller is only the Driver corresponding to the driverId

    function _assertCallerIsDriver(uint32 driverId) internal view {
        require(driverAddress(driverId) == msg.sender, "Callable only by the driver");
    }

In other words, only AddressDriver/NFTDriver can call DripsHub.updateDriverAddress, but this function is not implemented in AddressDriver/NFTDriver. In the test, vm.prank(driver) is used, which cannot be done in the mainnet.

    function testUpdateDriverAddress() public {
        assertEq(driver, dripsHub.driverAddress(driverId), "Invalid driver address before");
        address newDriverAddr = address(0x1234);
        vm.prank(driver);
        dripsHub.updateDriverAddress(driverId, newDriverAddr);
        assertEq(newDriverAddr, dripsHub.driverAddress(driverId), "Invalid driver address after");
    }

This results in *Driver not being able to call DripsHub.updateDriverAddress to update the Driver address

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider implementing a function in *Driver that calls DripsHub.updateDriverAddress and adding access control to that function

[GalloDaSballo]

Will flag

[c4-judge]

GalloDaSballo marked the issue as primary issue

[xmxanuel]

This seems correct and the ownership of the AddressDriver and NFTDriver is not transferable.

I don't think this is a big problem the contract is still upgradable and we could add the function always in an upgrade if really needed.

[CodeSandwich]

[dispute validity] Our drivers don't use updateDriverAddress because they are upgradeable and they don't need to do that. This function is intended for 3rd party drivers, which may not want to follow the upgradeable proxy pattern and are fine with changing their addresses. If our drivers ever need a functionality like this, we may upgrade them into versions capable of calling updateDriverAddress on DripsHub.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

I believe that because of the contest scope, per this rule: We cannot judge the finding based on future code, that said the Sponsor is free to mitigate the finding via upgrades

[c4-judge]

GalloDaSballo marked the issue as selected for report

[xmxanuel]

Why is this one marked as selected for the report?

[0xA5DF]

I don't understand how this is a valid issue:

• Devs made an interface for drivers and added the optional updateDriverAddress() function
• Devs also added some drivers implementation, in those implementations they chose not to use updateDriverAddress() because it's not needed in that case (they upgrade via the proxy rather than by migrating to another address)

This seems like everything is perfectly fine. Additionally, what's the impact of this issue? What part of the protocol doesn't work as intended due to this issue?

[GalloDaSballo]

After clarification, am downgrading this finding to QA as it doesn't show a vulnerability but is rather a QA finding

L

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[CodeSandwich]

I too think that it's not an issue and definitely not a good candidate for the report. It'll make C4 look bad if you include a non-issue like this.

[c4-judge]

GalloDaSballo marked the issue as not selected for report

[c4-judge]

GalloDaSballo marked the issue as grade-c