c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/CreateOffererLib.sol#L145-L149 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/CreateOffererLib.sol#L169-L172 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/CreateOfferer.sol#L52-L80

Vulnerability details

Impact

The onlySeaport holds a lot of power within the system, which can compromise the system integrity and it's permission-less nature.

Having a single EOA as onlySeaport is a large centralization risk and a single point of failure. A single private key may be taken in a hack, or the sole holder of the key may become unable to retrieve the key when necessary. Consider changing to a multi-signature setup, or having a role-based authorization model.

The very similar issues had been evaluated during a previous Code4rena audit as an issue with Medium severity:

Proof of Concept

The onlySeaport is responsible for generating and ratifying orders:

function generateOrder(address fulfiller, SpentItem[] calldata minimumReceived, SpentItem[] calldata maximumSpent, bytes calldata context)
function ratifyOrder(SpentItem[] calldata offer, ReceivedItem[] calldata consideration, bytes calldata context, bytes32[] calldata, uint256 contractNonce)

Those functions implement onlySeaport modifier, which looks like below:

    modifier onlySeaport(address caller) {
        if (caller != seaport) revert CreateOffererErrors.CallerNotSeaport(caller);
        _;
    }

The seaport address is set in constructor and is a single EOA:

    constructor(address setSeaport, CreateOffererEnums.Stage firstStage) {
        if (setSeaport == address(0)) revert CreateOffererErrors.SeaportIsZero();
        seaport = setSeaport;
        stage = CreateOffererStructs.Stage({flag: firstStage, lock: CreateOffererEnums.Lock.unlocked});
    }

Whenever this account became compromised or access to that account was lost - protocol wouldn't be operating properly - as it wouldn't be possible to generate any new orders or ratify them (functions generateOrder and ratifyOrder are callable only by seaport account).

Tools Used

Manual code review

Recommended Mitigation Steps

Without significant redesign it is not possible to avoid centralization risk. Consider changing to a multi-signature setup (at least a few accounts should have onlySeaport role - in case access to one of them would be lost). Moreover, some authorization model should be implemented. There need to be a chance to revoke onlySeaport role of potentially compromised accounts.

Assessed type

Other

GalloDaSballo commented 11 months ago

Flagging but looks wrong Seaport is a contract of which the implementation is known

c4-sponsor commented 11 months ago

0xfoobar (sponsor) disputed

c4-judge commented 11 months ago

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof

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

The `onlySeaport` is a single point of failure and a centralization risk #316