Tapioca DAO - cergyk's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L288 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#L550-L552

Vulnerability details

Impact

Malicious user can hijack approvals to BigBang or Singularity from any other user

Proof of Concept

We can see that BigBang and Singularity have a addCollateral function accepting a from parameter.

Although it checks if msg.sender has allowance for the shares of collateral to add, by using the allowedBorrow modifier:

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

It does not check if msg.sender has the additional allowance for the amount requested. in the _addCollateral function, if shares == 0 the amount of shares is set to be the amount:

function _addCollateral(
    address from,
    address to,
    bool skim,
    uint256 amount,
    uint256 share
) internal {
    if (share == 0) {
        share = yieldBox.toShare(collateralId, amount, false);
    }
    userCollateralShare[to] += share;
    uint256 oldTotalCollateralShare = totalCollateralShare;
    totalCollateralShare = oldTotalCollateralShare + share;
    _addTokens(from, collateralId, share, oldTotalCollateralShare, skim);
    emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
}

So a malicious user Alice can simply call:

BigBang.addCollateral(
    Bob,
    Alice,
    false,
    big_amount,
    0
)

to hijack the approval that Bob made to the BigBang/Singularity contract and add collateral for her account.

Tools Used

Manual review

Recommended Mitigation Steps

Check also the allowance for the amount variable, not only shares

Assessed type

Access Control

Lines of code

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L118-L160 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L102-L104 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L130-L134

Vulnerability details

Impact

The function updateCache() in BalancerStrategy is manipulatable and not rate limited, which can enable transaction sandwiching in a safe way for a mev bot.

Proof of Concept

Since the function BalancerStrategy::updateCache() sets in storage the price to be used for the next deposit/withdraw/emergencyWithdraw, a sandwich bot Alice can do the following:

• Manipulate the balancer tricrypto pool to increase price of ETH in the tricrypto pool
• Call BalancerStrategy::updateCache()
• Manipulate the balancer tricrypto pool back to normal price

Bob deposits and receives less shares than due, due to reduced _tokenBalanceOf in YieldBox:

function depositAsset(
    uint256 assetId,
    address from,
    address to,
    uint256 amount,
    uint256 share
) public allowed(from, assetId) returns (uint256 amountOut, uint256 shareOut) {
    // Checks
    Asset storage asset = assets[assetId];
    require(asset.tokenType != TokenType.Native && asset.tokenType != TokenType.ERC721, "YieldBox: can't deposit type");

    // Effects
    uint256 totalAmount = _tokenBalanceOf(asset);
    if (share == 0) {
        // value of the share may be lower than the amount due to rounding, that's ok
        share = amount._toShares(totalSupply[assetId], totalAmount, false);
    } else {
        // amount may be lower than the value of share due to rounding, in that case, add 1 to amount (Always round up)
        amount = share._toAmount(totalSupply[assetId], totalAmount, true);
    }

    _mint(to, assetId, share);

    //...Rest of the function
}

because _tokenBalanceOf in turn calls strategy._currentBalance():

function _tokenBalanceOf(Asset storage asset) internal view returns (uint256 amount) {
    return asset.strategy.currentBalance();
}

And BalancerStrategy::currentBalance uses _cachedCalculatedAmount:

function _currentBalance() internal view override returns (uint256 amount) {
    uint256 queued = wrappedNative.balanceOf(address(this));

    return _cachedCalculatedAmount + queued;
}

Tools Used

Manual review

Recommended Mitigation Steps

Make updateCache() private on BalancerStrategy

Assessed type

MEV

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L103-L114 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L225-L230 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L134-L143 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125

Vulnerability details

Impact

For some strategies, the value returned by _currentBalance() is manipulatable in a given block, which in turn may lead to the manipulation of the share price for yieldboxes.

The vulnerable strategies are the ones relying on a spot price to evaluate current balance:

• AaveStrategy
• ConvexTricryptoStrategy
• StargateStrategy
• LidoEthStrategy (curve spot price get_dy)

Since the share/amount ratio in yieldbox is calculated using strategy.currentBalance(), the manipulation of currentBalance results in the direct manipulation of share price in a yieldbox.

Proof of Concept

Let's take the example of AaveStrategy:

If there is a non zero claimable amount in compoundAmount():

function compoundAmount() public view returns (uint256 result) {
    uint256 claimable = stakedRewardToken.stakerRewardsToClaim(
        address(this)
    );
    result = 0;
    if (claimable > 0) {
        ISwapper.SwapData memory swapData = swapper.buildSwapData(
            address(rewardToken),
            address(wrappedNative),
            claimable,
            0,
            false,
            false
        );
        result = swapper.getOutputAmount(swapData, "");
        result = result - (result * 50) / 10_000; //0.5%
    }
}

a sandwich on the withdraw method associated with a yieldbox using this strategy can:

• Manipulate the pool rewardToken/wrappedNative to heavily inflate the price of the rewardToken
• Call to withdraw, with a share value small enough so the calculated inflated amount is lower than or equal to queued:

function _withdraw(
    address to,
    uint256 amount
) internal override nonReentrant {
    uint256 available = _currentBalance();
    require(available >= amount, "AaveStrategy: amount not valid");

    uint256 queued = wrappedNative.balanceOf(address(this));
    if (amount > queued) {
        compound("");

        uint256 toWithdraw = amount - queued;

        uint256 obtainedWrapped = lendingPool.withdraw(
            address(wrappedNative),
            toWithdraw,
            address(this)
        );
        if (obtainedWrapped > toWithdraw) {
            amount += (obtainedWrapped - toWithdraw);
        }
    }

    wrappedNative.safeTransfer(to, amount);
    emit AmountWithdrawn(to, amount);
}

• Manipulate the pool to return to real price

The attacker has withdrawn all the queued amount for less shares than needed.

Tools Used

Manual review

Recommended Mitigation Steps

Use a manipulation resistant price to evaluate _currentBalance, such as a TWAP or chainlink feed

Assessed type

Uniswap

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L180-L185

Vulnerability details

Impact

Tokens are not returned to caller during cross chain call if it fails. This can mean that these tokens are lost for the user if retrying the call cannot succeed.

Proof of Concept

In USDOLeverageModule, BaseTOFTLeverageModule, BaseTOFTStrategyModule in respectively leverageUp, leverageDown, strategyDeposit functions, we can see that if the call fails, tokens are attempted to be returned to the caller:

if (!success) {
    if (balanceAfter - balanceBefore >= amount) {
        IERC20(address(this)).safeTransfer(leverageFor, amount);
    }
    revert(_getRevertMsg(reason)); //forward revert because it's handled by the main executor
}

However since the call always reverts right after the transfer, funds are never returned to the user

Tools Used

Manual review

Recommended Mitigation Steps

do not revert the call, return instead

Assessed type

call/delegatecall

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L169 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOMarketModule.sol#L168 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOOptionsModule.sol#L174 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L184

Vulnerability details

Impact

Unchecked delegate calls may be used to deactivate libraries and leave bricked funds

Proof of Concept

Some contracts have unchecked delegate calls to arbitrary addresses: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOLeverageModule.sol#L169

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOMarketModule.sol#L168

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/modules/USDOOptionsModule.sol#L174

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L184

Since the function is unpermissioned and module is an unchecked argument, a user can create a contract with a function using the selfdestruct opcode and call it using this delegate call. This will leave the modules contract without any code, and brick the funds of the contracts relying on it.

Please note that eip-4758 is planned to remove the selfdestruct opcode, but since it is not yet implemented, defense against this is advised.

Tools Used

Manual review

Recommended Mitigation Steps

Please add a check on this module argument, or make it a immutable value in the contract

Assessed type

call/delegatecall

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/oracle/implementations/ARBTriCryptoOracle.sol#L118

Vulnerability details

Impact

Read-only reentrancy makes ARBTriCryptoOracle vulnerable to manipulation

Proof of Concept

When getting the price, ARBTriCryptoOracle makes use of TRI_CRYPTO.get_virtual_price(), which is vulnerable to the read-only reentrancy attack well documented here: https://chainsecurity.com/curve-lp-oracle-manipulation-post-mortem/

Here are the impacted code lines:

function _get() internal view returns (uint256 _maxPrice) {
>>> uint256 _vp = TRI_CRYPTO.get_virtual_price();

    // Get the prices from chainlink and add 10 decimals
    uint256 _btcPrice = uint256(BTC_FEED.latestAnswer()) * 1e10;
    uint256 _wbtcPrice = uint256(WBTC_FEED.latestAnswer()) * 1e10;
    uint256 _ethPrice = uint256(ETH_FEED.latestAnswer()) * 1e10;
    uint256 _usdtPrice = uint256(USDT_FEED.latestAnswer()) * 1e10;

    uint256 _minWbtcPrice = (_wbtcPrice < 1e18)
        ? (_wbtcPrice * _btcPrice) / 1e18
        : _btcPrice;

    uint256 _basePrices = (_minWbtcPrice * _ethPrice * _usdtPrice);

>>> _maxPrice = (3 * _vp * FixedPointMathLib.cbrt(_basePrices)) / 1 ether;

    // ((A/A0) * (gamma/gamma0)**2) ** (1/3)
    uint256 _g = (TRI_CRYPTO.gamma() * 1 ether) / GAMMA0;
    uint256 _a = (TRI_CRYPTO.A() * 1 ether) / A0;
    uint256 _discount = Math.max((_g ** 2 / 1 ether) * _a, 1e34); // handle qbrt nonconvergence
    // if discount is small, we take an upper bound
    _discount = (FixedPointMathLib.sqrt(_discount) * DISCOUNT0) / 1 ether;

    _maxPrice -= (_maxPrice * _discount) / 1 ether;
}

Tools Used

Manual review

Recommended Mitigation Steps

Make a call to remove_liquidity with 0 value in order to trigger nonReentrant modifier on the curve pool

Assessed type

Reentrancy

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L336-L376 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/Singularity.sol#L351-L373 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLeverage.sol#L147-L187

Vulnerability details

Impact

buyCollateral in Singularity and BigBang can be abused to steal users collateral

Proof of Concept

In the function Singularity.buyCollateral, we can see:

function buyCollateral(
    address from,
    uint256 borrowAmount,
    uint256 supplyAmount,
    uint256 minAmountOut,
    ISwapper swapper,
    bytes calldata dexData
) external notPaused solvent(from) returns (uint256 amountOut) {
    require(penrose.swappers(swapper), "SGL: Invalid swapper");

    // Let this fail first to save gas:
    uint256 supplyShare = yieldBox.toShare(assetId, supplyAmount, true);
    if (supplyShare > 0) {
        yieldBox.transfer(from, address(swapper), assetId, supplyShare);
    }

    uint256 borrowShare;
    (, borrowShare) = _borrow(from, address(swapper), borrowAmount);

    ISwapper.SwapData memory swapData = swapper.buildSwapData(
        assetId,
        collateralId,
        0,
        supplyShare + borrowShare,
        true,
        true
    );

    uint256 collateralShare;
    (amountOut, collateralShare) = swapper.swap(
        swapData,
        minAmountOut,
        from,
        dexData
    );
    require(amountOut >= minAmountOut, "SGL: not enough");

    _allowedBorrow(from, collateralShare);
    _addCollateral(from, from, false, 0, collateralShare);
}

That _allowedBorrow(from, collateralShare) is only checked after the swap. Which means that anyone can call this function with minAmountOut == 0, extract equivalent to supplyShare + borrowShare by sandwiching the swap (or using a malicious path), and since returned collateralShare == 0, _allowedBorrow checks out even if from did not approve msg.sender for any amount

This means msg.sender stole an arbitrary amount from from, with the constraint however, that from remains solvent after the call (supplyShare + borrowShare value must be adjusted accordingly).

Tools Used

Manual review

Recommended Mitigation Steps

Make the check on allowance first, by computing the conversion of supplyShare + borrowShare exchange rate into collateral (with acceptable slippage).

Assessed type

Access Control

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLiquidation.sol#L133 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L819

Vulnerability details

Impact

Total collateral is not updated during liquidation in BigBang.sol and Singularity.sol. This can mean that the minting of shares is broken, and a user minting after a liquidation has taken place can receive less shares than due.

Proof of Concept

We can see in the liquidation functions of BigBang and Singularity that although user collateral shares are reduced: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLiquidation.sol#L133

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L819

totalCollateralShare is not reduced accordingly.

This means that skim based adding collateral will be DOSed because totalCollateralShare will always cause an underflow here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L700

Note that this is correctly done in the particular case of the order book flow in Singularity: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLiquidation.sol#L157

Here all variables are correctly updated

Tools Used

Manual review

Recommended Mitigation Steps

reduce totalCollateralShare when updating individual collateral shares

Assessed type

Under/Overflow

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L280-L295

Vulnerability details

Impact

Balancer::rebalance will fail because msg.value is not set in _sendNative

Proof of Concept

We can see that _sendNative in Balancer.sol:

function _sendNative(
    address payable _oft,
    uint256 _amount,
    uint16 _dstChainId,
    uint256 _slippage
) private {
    if (address(this).balance < _amount) revert ExceedsBalance();

    routerETH.swapETH(
        _dstChainId,
        _oft, //refund
        abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
        _amount,
        _computeMinAmount(_amount, _slippage)
    );
}

Is supposed to swap native tokens (ETH) using routerETH. This call will always fail since msg.value is not set for the call

Tools Used

Manual review

Recommended Mitigation Steps

Consider using instead:

function _sendNative(
    address payable _oft,
    uint256 _amount,
    uint16 _dstChainId,
    uint256 _slippage
) private {
    if (address(this).balance < _amount) revert ExceedsBalance();

    routerETH.swapETH{value: msg.value}(
        _dstChainId,
        _oft, //refund
        abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
        _amount,
        _computeMinAmount(_amount, _slippage)
    );
}

Assessed type

call/delegatecall

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L224-L231

Vulnerability details

Impact

oTAP::participate call will always revert if msg.sender is approved but not owner

Proof of Concept

We can see in the implementation of oTAP::participate, the following lines are used to ensure caller has the correct access control wrt to the _tOLPTokenID:

require(
    tOLP.isApprovedOrOwner(msg.sender, _tOLPTokenID),
    "tOB: Not approved or owner"
);

// Transfer tOLP position to this contract
tOLP.transferFrom(msg.sender, address(this), _tOLPTokenID);

However this will always fail if msg.sender is not the owner of the token, since tOLP.transferFrom(msg.sender, address(this), _tOLPTokenID); will revert in that case.

This can break interoperability with operators supposed to be able to execute actions on behalf of a user such as MagnetarV2 or tOFT.

Tools Used

Manual review

Recommended Mitigation Steps

Transfer from the owner:

    address owner = tOLP.ownerOf(_tOLPTokenID);
    tOLP.transferFrom(owner, address(this), _tOLPTokenID);

Assessed type

Access Control

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L413-L432

Vulnerability details

Impact

In TapiocaOptionBroker an epoch can be skipped leading for unclaimed tap to distribute to be lost

Proof of Concept

The function newEpoch is called to start a new epoch:

function newEpoch() external {
    require(
        block.timestamp >= lastEpochUpdate + EPOCH_DURATION,
        "tOB: too soon"
    );
    uint256[] memory singularities = tOLP.getSingularities();
    require(singularities.length > 0, "tOB: No active singularities");

    // Update epoch info
    lastEpochUpdate = block.timestamp;
    epoch++;

    // Extract TAP
    uint256 epochTAP = tapOFT.emitForWeek();
    _emitToGauges(epochTAP);

    // Get epoch TAP valuation
    (, epochTAPValuation) = tapOracle.get(tapOracleData);
    emit NewEpoch(epoch, epochTAP, epochTAPValuation);
}

As we can see that lastEpochUpdate is set to be block.timestamp which can be slightly bigger than lastEpochUpdate + EPOCH_DURATION, which means that the transition from one epoch to another lags slightly behind a strict weekly schedule.

Inversely, in tapOFT.emitForWeek():

function emitForWeek() external notPaused returns (uint256) {
    require(_getChainId() == governanceChainIdentifier, "chain not valid");

>>> uint256 week = _timestampToWeek(block.timestamp);
    if (emissionForWeek[week] > 0) return 0;

    // Update DSO supply from last minted emissions
    dso_supply -= mintedInWeek[week - 1];

    // Compute unclaimed emission from last week and add it to the current week emission
    uint256 unclaimed = emissionForWeek[week - 1] - mintedInWeek[week - 1];
    uint256 emission = uint256(_computeEmission());
    emission += unclaimed;
    emissionForWeek[week] = emission;

    emit Emitted(week, emission);

    return emission;
}

_timestampToWeek function is used, which enforces a strict weekly schedule for epochs:

function _timestampToWeek(
    uint256 timestamp
) internal view returns (uint256) {
    return ((timestamp - emissionsStartTime) / WEEK) + 1; // Starts at week 1
}

Since the variable lastEpochUpdate is updated with an interval slightly bigger than 1 WEEK, the drift between _timestampToWeek and lastEpochUpdate with regards to WEEK grows over time.

At some point, when this remainder grows close enough to 1 WEEK (let's say 1 WEEK - 5 seconds for the sake of the example):

• the operator calls newEpoch, _timestampToWeek returns epoch N
• wait 1 WEEK + 10 seconds
• operator calls newEpoch, _timestampToWeek returns epoch N+2

An epoch is skipped in the process. Since the logic relies on transferring values from one epoch to another, this means that some rewards become unclaimable:

    // Compute unclaimed emission from last week and add it to the current week emission
    uint256 unclaimed = emissionForWeek[week - 1] - mintedInWeek[week - 1];
    uint256 emission = uint256(_computeEmission());
    emission += unclaimed;

Tools Used

Manual review

Recommended Mitigation Steps

Use the same fixed weekly schedule to determine if newEpoch can be called: Change the condition to: require( _timestampToWeek(block.timestamp) > lastEpochUpdate, "tOB: too soon" ); And assignement to: lastEpochUpdate = _timestampToWeek(block.timestamp);

Assessed type

Timing

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L339 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/modules/MagnetarMarketModule.sol#L542-L549

Vulnerability details

Impact

In MagnetarV2, exitPositionAndRemoveCollateral cannot be used to exitPosition without unlocking tOlp

Proof of Concept

in MagnetarMarketModule.sol, in the function _exitPositionAndRemoveCollateral, we can see that in the case where `` is true:

ITapiocaOptionsBroker(removeAndRepayData.exitData.target)
    .exitPosition(removeAndRepayData.exitData.oTAPTokenID);

if (!removeAndRepayData.unlockData.unlock) {
    IERC721(oTapAddress).safeTransferFrom(
        address(this),
        user,
        removeAndRepayData.exitData.oTAPTokenID,
        "0x"
    );
}

if removeAndRepayData.unlockData.unlock == false, oTAPTokenID is transferred back to the user. This will always revert since the TapiocaOptionsBroker always burns the token when exiting position:

    address otapOwner = oTAP.ownerOf(_oTAPTokenID);
    delete participants[oTAPPosition.tOLP];
    oTAP.burn(_oTAPTokenID);

Tools Used

Manual review

Recommended Mitigation Steps

The tOlp token should instead be sent back to the user

Assessed type

Error

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L394 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L417-L419

Vulnerability details

Impact

In both Singularity and BigBang, the remainder of removed collateral is never returned to the user in sellCollateral after repayment of the loan

Proof of Concept

A user can call sellCollateral with a bigger share than the amount she borrowed.

The whole collateral share is first removed:

_removeCollateral(from, address(swapper), share);

The swap determines the shareOut value, and then this condition is executed:

if (shareOwed <= shareOut) {
    _repay(from, from, partOwed);
}

Which means that only the part borrowed by the user is repaid, and the rest of asset obtained after swap only stays in BigBang. This means that the user actually lost the remainder of what was swapped, and the call does not revert unless the user is not solvent

Tools Used

Manual review

Recommended Mitigation Steps

Add the surplus of asset to the balance of the user selling collateral

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L347-L355 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L303-L306 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L358-L359 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L216-L219

Vulnerability details

Impact

Repeated oTap generation for one tOLP is possible when block.timestamp == lock.lockTime + lock.lockDuration. This means a malicious user can take a disproportionate share of TAP tokens and in turn manipulate governance of the protocol

Proof of Concept

TapiocaOptionBroker handles the locking of tOLP tokens in order to mint a oTap (option to buy TAP tokens). Inversely, it is possible to unlock the tOLP token when expired in order to unlock underlying singularity assets.

However, we can see that the condition for tOLP to be considered active is:

function _isPositionActive(uint256 tokenId) internal view returns (bool) {
    LockPosition memory lockPosition = lockPositions[tokenId];

    return
        (lockPosition.lockTime + lockPosition.lockDuration) >=
        block.timestamp;
}

And also the condition for the tOLP to be considered unlocked in TapiocaOptionBroker is:

function exitPosition(uint256 _oTAPTokenID) external {
    require(oTAP.exists(_oTAPTokenID), "tOB: oTAP position does not exist");

    // Load data
    (, TapOption memory oTAPPosition) = oTAP.attributes(_oTAPTokenID);
    (, LockPosition memory lock) = tOLP.getLock(oTAPPosition.tOLP);

    require(
>>>     block.timestamp >= lock.lockTime + lock.lockDuration,
        "tOB: Lock not expired"
    );
    //... Some code
}

These conditions are not exclusive, and it means that when block.timestamp == lock.lockTime + lock.lockDuration, So a malicious user Alice can execute following steps to repeatedly exercise options when block.timestamp == lock.lockTime + lock.lockDuration:

• call TapiocaOptionBroker::participate to use the tOLP position to mint a oTap
• call TapiocaOptionBroker::exerciseOption to exercise the oTap option
• call TapiocaOptionBroker::exitPosition to burn the oTap token

on step3 oTLP position is transferred back to Alice, so Alice can start again at step 1, until the gauge limit is reached for that epoch.

Tools Used

Manual review

Recommended Mitigation Steps

Use exclusive conditions, e.g. block.timestamp < lock.lockTime + lock.lockDuration for verifying a position is active and block.timestamp >= lock.lockTime + lock.lockDuration to verify the position is unlocked

Assessed type

Timing

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L252-L348

Vulnerability details

Impact

twTap voting power is subject to manipulation when locking tokens via twTap::participate

Indeed magnitude calculations do not include the amount of tokens locked and allow for exceedingly high _duration values (close to type(uint56).max) A user can lock minimal amount tokens and a very high _duration, and have the same influence on the variables pool.cumulative, pool.averageMagnitude as a user locking more tokens,

The minimum amount is only constrained to be:

bool hasVotingPower = _amount >=
    computeMinWeight(pool.totalDeposited, MIN_WEIGHT_FACTOR);

e.g greater than 0.01% of totalDeposited

Proof of Concept

Let's consider the following scenario: totalDeposited is 10000 pool.averageMagnitude is 1 WEEK = 604800 pool.cumulative is also 1 WEEK number of participants: 10

1/ Alice deposits 20 of TAP (just above the threshold 10000*0.001)

with a _duration == type(uint56).max-block.timestamp-1 such as the condition at lines 312-313:

    uint256 expiry = block.timestamp + _duration;
    require(expiry < type(uint56).max, "twTAP: too long");

is verified.

At current time of writing such a value for _duration would be around 72057592347712460.

so new state is: totalDeposited: 10020 magnitude: 72057592347712460 pool.averageMagnitude is 6550690213978224 pool.cumulative becomes 6550690214583024 number of participants: 11

2/ Alice deposits again 20 of TAP this time with a _duration == 1 WEEK (minimal)

new state is: totalDeposited: 10040 magnitude: 0 pool.averageMagnitude is 6004799362813372 pool.cumulative becomes 545890851769652 number of participants: 12

3/ Alice does exactly the same

new state is: totalDeposited: 10060 magnitude: 0 pool.averageMagnitude is 5542891719520036 pool.cumulative becomes 0 number of participants: 13

As a side note this proves the TODO remark incorrect:

// TODO: Strongly suspect this is never less. Prove it.
if (pool.cumulative > pool.averageMagnitude) {

Now alice can lock the real amount of token she wants to mint because multiplier is maximal if pool.cumulative == 0

It is trivial to see that Alice can also make the value very large and grief other users of the protocol.

Tools Used

Manual review

Recommended Mitigation Steps

Weigh in the amount locked in the magnitude calculation, otherwise it is too easily manipulatable

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L192-L194 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/UniswapV2Swapper.sol#L58-L76

Vulnerability details

Impact

AaveStrategy computes slippage protection on spot price, can lead to unlimited slippage

Proof of Concept

In AaveStrategy, the minAmountOut value computed for slippage protection is computed on spot price if swapper is UniswapV2Swapper for example:

AaveStrategy::compound():

uint256 calcAmount = swapper.getOutputAmount(swapData, "");
uint256 minAmount = calcAmount - (calcAmount * 50) / 10_000; //0.5%
swapper.swap(swapData, minAmount, address(this), "");

If swapper is UniV2Swapper:

function getOutputAmount(
    SwapData calldata swapData,
    bytes calldata
) external view override returns (uint256 amountOut) {
    (address tokenIn, address tokenOut) = _getTokens(
        swapData.tokensData,
        yieldBox
    );
    address[] memory path = _createPath(tokenIn, tokenOut);
    (uint256 amountIn, ) = _getAmounts(
        swapData.amountData,
        swapData.tokensData.tokenInId,
        swapData.tokensData.tokenOutId,
        yieldBox
    );

    uint256[] memory amounts = swapRouter.getAmountsOut(amountIn, path);
    amountOut = amounts[1];
}

getOutputAmount returns spot price, which is very easily manipulatable. If a malicious user has manipulated the pool before calling compound(), as is the case during a sandwich, the slippage protection computed here is useless.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that a reliable price source is used to compute slippage protection in that case, such as a chainlink feed

Assessed type

MEV