code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

Sweep to ZERO address from DelegateRegistry #359

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/delegatexyz/delegate-registry/blob/6d1254de793ccc40134f9bec0b7cb3d9c3632bc1/src/DelegateRegistry.sol#L151-L158

Vulnerability details

Impact

The sweep function is used to sweep every token in the Registry to a hardcoded address. However there is an issue. The address which is being swept to is address(0). The code highlights it as something to be changed. However this was not listed under the known issues, therefore it is an oversight here as any time sweep is called, it gets sent to address(0)

Proof of Concept

   function sweep() external {
        // TODO: Replace this with correct address at deployment time
        // This hardcoded address is a CREATE2 factory counterfactual smart contract wallet that will always accept native token transfers
        uint256 sc = uint256(uint160(0x0000000000000000000000000000000000000000));
        assembly ("memory-safe") {
            let result := call(gas(), sc, selfbalance(), 0, 0, 0, 0)
        }
    }

The hardcoded address is the zero address. Therefore all swept funds go to address(0)

Tools Used

Manual Review

Recommended Mitigation Steps

Use a proper address as the token receiver.

Assessed type

ETH-Transfer

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

GalloDaSballo commented 1 year ago

IMO QA Severity, especially since it's commented in the code

c4-sponsor commented 1 year ago

0xfoobar (sponsor) disputed

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by GalloDaSballo

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c

Josephdara commented 1 year ago

It is believed that the code being audited is a snapshot of deployment ready code, the TODO might have as well been forgotten as we have seen code being deployed without all Todo's being cleared. Therefore at the snapshot of this code, it does sweep to address(0)