code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Multicall/Multicall3.sol#L41-L61

Vulnerability details

Impact

The Multicall3.multicall(...) method can be invoked with an array of the Call struct

    struct Call {
        address target;
        bool allowFailure;
        bytes callData;
    }

and returns an array of the Result struct

    struct Result {
        bool success;
        bytes returnData;
    }

which unambiguously shows the intent to allow failed calls (core functionality) since the user is able to specify bool allowFailure and gets bool success in return.

However, the Multicall3.multicall(...) method completely ignores the value of allowFailure and reverts on any failed call leading to unexpected DoS for the user of Multicall3. Furthermore, the success return value becomes unnecessary since it could only be true in the non-reverting case.

Note that the same isse is true for Multicall3.multicallValue(...), but in case of correctly implementing allowFailure, the calli.value needs to be refunded to the msg.sender in order to avoid irrecoverably stuck funds. Otherwise, allowing transactions with calli.value close to type(uint256).max to fail without refund would facilitate an overflow of valAccumulator (e.g. reset to 0, allowing msg.value == 0) due to its unchecked accumulation.

Proof of Concept

The following PoC is based on two existing test cases which expect the multicall to revert on failure. Even when setting allowFailure to true, the multicall still reverts in both cases and therefore demonstrates the issue.

Just apply the diff below in tapioca-periph-audit and run the test cases with npx hardhat test test/multicall.test.ts:

diff --git a/test/multicall.test.ts b/test/multicall.test.ts
index b0444c0..db22339 100644
--- a/test/multicall.test.ts
+++ b/test/multicall.test.ts
@@ -33,7 +33,7 @@ describe('Multicall test', () => {

             calls.push({
                 target: revertContract.address,
-                allowFailure: false,
+                allowFailure: true,
                 callData,
             });
             await expect(multiCall.multicall(calls)).to.be.revertedWith(
@@ -81,7 +81,7 @@ describe('Multicall test', () => {
             const calls: Multicall3.CallStruct[] = [];
             calls.push({
                 target: tapiocaDeployer.address,
-                allowFailure: false,
+                allowFailure: true,
                 callData,
             });

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

Do not revert in Multicall3._getRevertMsg(...) and actually store the revert message in the returnData instead.
Back in Multicall3.multicall(...), decide according to allowFailure whether to revert or not.
Be careful concerning stuck funds in Multicall3.multicallValue(...), see the note above.

Assessed type

DoS

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

0xRektora marked the issue as disagree with severity

c4-sponsor commented 1 year ago

0xRektora marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)