2024-12-28
*issue*
================================================================================

Malicious actor can DoS admin functions due to lack of access control

================================================================================

*issue-contents*

0 CONTENTS

*issue-metadata*

1 METADATA

Number 521
Severity Medium
Author tjonair
Contest Autonomint
Platform Sherlock
*issue-summary*

2 SUMMARY

The multiSign::approveSetterFunction is used by owners to approve specific setter functions of their choice which allow the admin to set critical admin functions in borrowing and CDS contracts.

    function approveSetterFunction(
        uint8[] memory functions
    ) external onlyOwners {

The multiSign::executeSetterFunction is used by the admin functions of borrowing and CDS contracts to check the approval first to allow updating certain functionalities as well as set the approvals back to false.

        // Example of how this function is being called in both contracts
        if (!multiSign.executeSetterFunction(IMultiSign.SetterFunctions(2))) revert CDS_RequiredApprovalsNotMetToSet();

However, this function is not restricted to borrowing and CDS contracts due to lack of access control.

    function executeSetterFunction(
        SetterFunctions _function
    ) external returns (bool) {          <@ - // Lack of access control
        // Get the number of approvals
        require(getSetterFunctionApproval(_function) >= requiredApprovals,"Required approvals not met");
        // Loop through the approved functions with owners mapping and change to false
        for (uint64 i; i < noOfOwners; i++) {
            approvedToUpdate[_function][owners[i]] = false;
        }
        return true;
    }

Hence, an attacker can deny the admin from updating critical admin functionalities in borrowing and CDS contracts.

*issue-root-cause*

3 ROOT CAUSE

In multiSign.sol:306, there is lack of access control which would allow anyone to call executeSetterFunction and reset the approvedToUpdate values to false.

*issue-internal-pre-conditions*

4 INTERNAL PRE-CONDITIONS

No response

*issue-external-pre-conditions*

5 EXTERNAL PRE-CONDITIONS

No response

*issue-attack-path*

6 ATTACK PATH

  1. Any of the owners in the multiSign contract calls approveSetterFunction to provide approvals for updation of a certain function.
  2. Malicious actor keeps on checking the values of approvedToUpdate for all owners (which will be less in number) and at any time if it turns out to be true, he can call the executeSetterFunction to deny the owner from calling update functions in CDS and borrowing contracts.
*issue-impact*

7 IMPACT

  1. DoS of a plethora of admin functions in both CDS and borrowing contracts.
*issue-poc*

8 POC

Add the following test inside the BorrowingTest.ts:

describe("Testing Admin functions", async () => {
    it("Should allow anyone to set all approvedToUpdate to false", async () => {
        const { multiSignA } = await loadFixture(deployer);

        // Owner1 and Owner2 approve to update 1 and 2
        await multiSignA.connect(owner1).approveSetterFunction([1, 2, 0]);
        await multiSignA.connect(owner2).approveSetterFunction([1, 2, 0]);

        expect(
            await multiSignA.approvedToUpdate(1, await owner1.getAddress())
        ).to.be.equal(true);
        expect(
            await multiSignA.approvedToUpdate(2, await owner2.getAddress())
        ).to.be.equal(true);

        // Anyone can set all approvedToUpdate to false
        await multiSignA.connect(user1).executeSetterFunction(1);
        await multiSignA.connect(user1).executeSetterFunction(2);

        expect(
            await multiSignA.approvedToUpdate(1, await owner1.getAddress())
        ).to.be.equal(false);
        expect(
            await multiSignA.approvedToUpdate(2, await owner2.getAddress())
        ).to.be.equal(false);
    });
});
*issue-mitigation*

9 MITIGATION

It is recommended to consider adding a onlyCoreContracts modifier to executeSetterFunction.

================================================================================

LINKS

*issue-links*