csanuragjain - ACL missing on init function

[sherlock-admin]

csanuragjain

medium

ACL missing on init function

Summary

The init method can be called by anyone to reset system state.

Vulnerability Detail

1. Observe the init function

function init() external {
        // adding ERC165 data
        LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
        ds.supportedInterfaces[type(IERC165).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondCut).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondLoupe).interfaceId] = true;
        ds.supportedInterfaces[type(IERC173).interfaceId] = true;

        // initializing protocol
        Protocol storage proto = protocolStorage();

        proto.tranche[0] = ONE.div(10).mul(4).div(365 days); // 40% APR
        proto.nbOfTranches = 1;
        proto.auction.priceFactor = ONE.mul(3);
        proto.auction.duration = 3 days;

        // initializing supply position nft collection
        SupplyPosition storage sp = supplyPositionStorage();
        sp.name = "Kairos Supply Position";
        sp.symbol = "KSP";
        ds.supportedInterfaces[type(IERC721).interfaceId] = true;
    }

2. Notice this function can be called by anyone and also resets all important variables to default value
3. Lets say if Admin has created a new tranche, this increases the proto.nbOfTranches to 2 using createTranche function

function createTranche(Ray newTranche) external onlyOwner returns (uint256 newTrancheId) {
        Protocol storage proto = protocolStorage();

        newTrancheId = proto.nbOfTranches++;
        proto.tranche[newTrancheId] = newTranche;

        emit NewTranche(newTranche, newTrancheId);
    }

4. Now Attacker can simply call init function and proto.nbOfTranches will be reset back to 1 even though it should be 2.
5. So now if Admin adds a new tranche it will overwrite his previous added tranche since proto.nbOfTranches++ will result in 2 instead of expected value of 3

Impact

Important variables like proto.nbOfTranches and auction parameters could be reset back to default value of 1. POC shows scenario where Attacker overwrites any tranche created by Admin

Code Snippet

https://github.com/sherlock-audit/2023-02-kairos/blob/main/kairos-contracts/src/Initializer.sol#L24

Tool used

Manual Review

Recommendation

Implement ACL on init function so that it is only callable by owner

[npasquie]

Diamond proxies call the init() function in the deploy transaction, and this function is inaccessible afterwards, this explains that ACL is not needed on init()

[csanuragjain]

Escalate for 10 USDC

This function remains accessible even after Diamond proxies initial call to init. We can see that in below method there is no way to stop calling it twice:

function init() external {
        // adding ERC165 data
        LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
        ds.supportedInterfaces[type(IERC165).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondCut).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondLoupe).interfaceId] = true;
        ds.supportedInterfaces[type(IERC173).interfaceId] = true;

        // initializing protocol
        Protocol storage proto = protocolStorage();

        proto.tranche[0] = ONE.div(10).mul(4).div(365 days); // 40% APR
        proto.nbOfTranches = 1;
        proto.auction.priceFactor = ONE.mul(3);
        proto.auction.duration = 3 days;

        // initializing supply position nft collection
        SupplyPosition storage sp = supplyPositionStorage();
        sp.name = "Kairos Supply Position";
        sp.symbol = "KSP";
        ds.supportedInterfaces[type(IERC721).interfaceId] = true;
    }

[sherlock-admin]

Escalate for 10 USDC

This function remains accessible even after Diamond proxies initial call to init. We can see that in below method there is no way to stop calling it twice:

function init() external {
        // adding ERC165 data
        LibDiamond.DiamondStorage storage ds = LibDiamond.diamondStorage();
        ds.supportedInterfaces[type(IERC165).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondCut).interfaceId] = true;
        ds.supportedInterfaces[type(IDiamondLoupe).interfaceId] = true;
        ds.supportedInterfaces[type(IERC173).interfaceId] = true;

        // initializing protocol
        Protocol storage proto = protocolStorage();

        proto.tranche[0] = ONE.div(10).mul(4).div(365 days); // 40% APR
        proto.nbOfTranches = 1;
        proto.auction.priceFactor = ONE.mul(3);
        proto.auction.duration = 3 days;

        // initializing supply position nft collection
        SupplyPosition storage sp = supplyPositionStorage();
        sp.name = "Kairos Supply Position";
        sp.symbol = "KSP";
        ds.supportedInterfaces[type(IERC721).interfaceId] = true;
    }

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[UsmannK]

Calling init() directly on the Initializer logic contract would not effect the protocol. The Initializer logic contract's storage contents do not matter.

To have an impact on the protocol you would need to call init() through the Diamond proxy, but it is not registered there so you cannot.

[hrishibhat]

Escalation rejected

Not a valid issue as pointed out in the comments above

[sherlock-admin]

Escalation rejected

Not a valid issue as pointed out in the comments above

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.