Tapioca DAO - LosPollosHermanos's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L381 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L414

Vulnerability details

Impact`

MasterContracts added in Penrose.addSingularity() and Penrose.addBigBang() forget to set masterContractOf[_contract] = mc. This will cause Penrose.executeMarketFn() to break if these contracts are called.

Proof of Concept

1. New market is registered via Penrose.addBigBang() or Penrose.addSingularity().
2. Someine calls Penrose.executeMarketFn() with a newly registered contract in a mc[] array.
3. The call fails here: https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L440 as master contract of contract registered in 1) is not defined.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Set masterContractOf[_contract] = mc in Penrose.addBigBang() and Penrose.addSingularity().

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L197-L203 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L258-L274

Vulnerability details

Impact

When a user tries to withdraw more funds than readily available from AaveStrategy using AaveStrategy._withdraw() (which is called through YieldBox.sol), AaveStrategy.compound() will wrongly try to deposit its funds through lendingPool.withdraw(). This creates an inconsistency between queued in AaveStrategy._withdraw() and the actual token balance of the contract, which leads to the contract attempting to send funds it will not have, therefore causing all withdrawals to revert.

Proof of Concept

1. Alice tries to withdraw 100 funds from AaveStrategy.sol through YieldBox.withdraw() which calls AaveStrategy.withdraw(). If Alice's withdrawal amount is larger than AaveStrategy's (we will use 50 as an example), then AaveStrategy.compound() will be called.
2. AaveStrategy will then try to claim rewardToken rewards through incentivesController.claimRewards(). If the number of rewards have increased, the if statement if (aaveBalanceAfter > aaveBalanceBefore) will pass, causing the contract to call lendingPool.deposit(). This will cause the token balance of the contract to decrease to 0, thereby leading to our inconsistency.
3. In _withdraw(), the contract will then withdraw amount - queued (100 - 50 = 50) and send amount (100) to the user. However, the contract will only have 50 tokens causing this transfer to revert.

Note that, withdrawals will only succeed if amount <= queued however queued can be artificially decreased by a malicious user frontrunning and calling AaveStrategy.deposited() so that queued > depositThreshold, thereby reducing queued to 0

Tools Used

VSCode

Recommended Mitigation Steps

Delete the block of code which deposits in compound(). Otherwise consider chainging compound() into a public and private function like so:

function compound(bytes memory data) external {
    // We want to deposit any rewards
    _compound(data, true);
}

function _compound(bytes memory, bool makeDeposit) internal {
    // Copy compound code here
    ...
    ...
    if (makeDeposit) {
        lendingPool.deposit(
                address(wrappedNative),
                queued,
                address(this),
                0
            );
            emit AmountDeposited(queued);
    }
}

and consider modifying AaveStrategy._withdraw() to:

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


        uint256 queued = wrappedNative.balanceOf(address(this));
        if (amount > queued) {
            // THIS LINE CHANGED
            _compound("", false);


            uint256 toWithdraw = amount - queued;


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


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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Penrose.withdrawAllMarketFees() can be called by anyone with arbitrary market data. This allows a malicious user to steal the majority of market fees.

When withdrawing non-WETH fees, a user can choose to swap in a low-liquidity pool, using a flash loan. They can drive the price of the non-WETH asset and as minAmountOut is user controlled. The protocol will take a large tank in protocol fees.

Proof of Concept

We assume that the fees are stored as USDC for simplicity

1. Annoying Alice takes a flash loan of USDC, she swaps in USDC for WETH into the UniswapV2 USDC-WETH pool, thereby driving the price of WETH up.

2. Alice now calls withdrawAllMarketFees() with minAmountOut equal to 0 and specifies UniswapV2 as the swapper (note UniswapV2 will most certainly be whitelisted, see UniswapV2Swapper.sol).

3. The protocol tries to swap USDC for WETH, increasing the price further and receiving very little WETH in return. As minAmountOut is zero, the swap will not revert and the protocol receives very little fees.

4. Alice then swaps her WETH back for USDC and makes profit due to the price increase (from the protocol swap) and pays back the flash loan.

5. This is basically what is known as a sandwich attack: https://medium.com/coinmonks/defi-sandwich-attack-explain-776f6f43b2fd. Except that no front-running is required for Alice, which makes this attack almost certainly profitable.

Severity

I have issued this vulnerability as high severity due to the fact that Uniswap V2 or V3 will be whitelisted swappers is almost certain combined with loss of protocol rewards.

Tools Used

VSCode

Recommended Mitigation Steps

Add the onlyOwner modifier to this function so that only trusted privileged users can run this function. Or consider using an oracle (chainlink will do) to calculate minAmountOut.

Assessed type

Uniswap