[HB10] `AaveStrategy.sol`: Changing swapper breaks the contract

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L129-L132

Vulnerability details

Impact

The contract AaveStrategy.sol manages wETH tokens and deposits them to the aave lending pool, and collects rewards. These rewards are then swapped into WETH again to compound on the WETH being managed by the contract. this is done in the compound function.

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

To carry out these operations, the swapper contract needs to be given approval to use the tokens being stored in the strategy contract. This is required since the swapper contract calls transferFrom on the tokens to pull it out of the strategy contract. This allowance is set in the constructor.

rewardToken.approve(_multiSwapper, type(uint256).max);

The issue arises when the swapper contract is changed. The change is done via the setMultiSwapper function. This function however does not give approval to the new swapper contract. Thus if the swapper is upgraded/changed, the approval is not transferred to the new swapper contract, which makes the swappers dysfunctional.

Since the swapper is critical to the system, and compound is called before withdrawals, a broken swapper will break the withdraw functionality of the contract. Thus this is classified as a high severity issue.

Proof of Concept

The bug is due to the absence of approve calls in the setMultiSwapper function. This can be seen from the implementation of the function.

function setMultiSwapper(address _swapper) external onlyOwner {
        emit MultiSwapper(address(swapper), _swapper);
        swapper = ISwapper(_swapper);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

In the setMultiSwapper function, remove approval from the old swapper and add approval to the new swapper. The same function has the proper implementation in the ConvexTricryptoStrategy.sol contract which can be used here as well.

function setMultiSwapper(address _swapper) external onlyOwner {
    emit MultiSwapper(address(swapper), _swapper);
    rewardToken.approve(address(swapper), 0);
    swapper = ISwapper(_swapper);
    rewardToken.approve(_swapper, type(uint256).max);
}

Assessed type

Context

[c4-pre-sort]

minhquanym marked the issue as duplicate of #332

[c4-pre-sort]

minhquanym marked the issue as duplicate of #222

[c4-judge]

dmvt marked the issue as selected for report