ctf_sec - Using space.balanceOf(address(this)) and balancer Vault balance to calculate spot price makes AutoRoller.sol very vulnerable to price manipulation.

[sherlock-admin]

ctf_sec

high

Using space.balanceOf(address(this)) and balancer Vault balance to calculate spot price makes AutoRoller.sol very vulnerable to price manipulation.

Summary

Using space.balanceOf(address(this)) and balancer Vault balance as spot price makes AutoRoller very vulnerable to manipulation.

Vulnerability Detail

Let us see how using space.balanceOf(address(this)) makes the code vulnerable to manipulation.

PreviewDeposit is used to calculate how much shares user can mint when user can deposit in Solmate implementation of ERC4626

    function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);

        afterDeposit(assets, shares);
    }

if the AutoRoller has maturity, the previewDeposit called is

    function previewDeposit(uint256 assets) public view override returns (uint256) {
        if (maturity == MATURITY_NOT_SET) {
            return super.previewDeposit(assets);
        } else {
            Space _space = space;
            (uint256 ptReserves, uint256 targetReserves) = _getSpaceReserves();

            // Calculate how much Target we'll end up joining the pool with, and use that to preview minted LP shares.
            uint256 previewedLPBal = (assets - _getTargetForIssuance(ptReserves, targetReserves, assets, adapter.scaleStored()))
                .mulDivDown(_space.adjustedTotalSupply(), targetReserves);

            // Shares represent proportional ownership of LP shares the vault holds.
            return previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this)));
        }
    }

see this line:

return previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this)));

If malicious actor send space token to this pool directly and imflate the _space.balanceOf(address(this))), the preview deposit would be truncated, even to 0, then user cannot mint share.

Another vulnerable-to-manipulation vector is that:

the totalAsset user balancer liquidity balance as the spot price to determine the total amount of assets. This is dangerous.

Space _space = space;
(uint256 ptReserves, uint256 targetReserves) = _getSpaceReserves();

(uint256 targetBal, uint256 ptBal, uint256 ytBal, ) = _decomposeShares(ptReserves, targetReserves, totalSupply, true);

which calls _getSpaceReserves

    /// @dev Get PT and Target reserve balances for the current Space pool.
    /// @return ptReserves PT reserve amount.
    /// @return targetReserves Target reserve amount.
    function _getSpaceReserves() internal view returns (uint256, uint256) {
        (, uint256[] memory balances, ) = balancerVault.getPoolTokens(poolId);
        uint256 _pti = pti;
        return (balances[_pti], balances[1 - _pti]);
    }

the ptReserves and targetReserves balance is returned.

and we calls

(uint256 targetBal, uint256 ptBal, uint256 ytBal, ) = _decomposeShares(ptReserves, targetReserves, totalSupply, true);

which calls:

    function _decomposeShares(uint256 ptReserves, uint256 targetReserves, uint256 shares, bool withLoose)
        internal view returns (uint256, uint256, uint256, uint256)
    {
        uint256 supply      = totalSupply;
        uint256 totalLPBal  = space.balanceOf(address(this));
        uint256 spaceSupply = space.adjustedTotalSupply();

        // Shares have a right to a portion of the PTs/asset floating around unencombered in this contract.
        return (
            shares.mulDivUp(totalLPBal.mulDivUp(targetReserves, spaceSupply) + (withLoose ? asset.balanceOf(address(this)) : 0), supply),
            shares.mulDivUp(totalLPBal.mulDivUp(ptReserves, spaceSupply) + (withLoose ? pt.balanceOf(address(this)) : 0), supply),
            shares.mulDivUp(yt.balanceOf(address(this)), supply),
            shares.mulDivUp(totalLPBal, supply)
        );
    }

note that space.balanceOf(address(this)) can inflated, user can also use flash loan to distort the liquidity in the balancer pool to manipulate targetReserves and ptReserves

then manipulate the spot price and the totalAssets() return value

            uint256 ptSpotPrice = _space.getPriceFromImpliedRate(
                (ptReserves + _space.adjustedTotalSupply()).divWadDown(targetReserves.mulWadDown(initScale)) - 1e18
            ); // PT price in Target.

            uint256 scale = adapter.scaleStored();

            if (ptBal >= ytBal) {
                // Target + combined PTs/YTs + PT spot value in Target.
                return targetBal + ytBal.divWadDown(scale) + ptSpotPrice.mulWadDown(ptBal - ytBal);
            } else {
                uint256 ytSpotPrice = (1e18 - ptSpotPrice.mulWadDown(scale)).divWadDown(scale);

                // Target + combined PTs/YTs + YT spot value in Target.
                return targetBal + ptBal.divWadDown(scale) + ytSpotPrice.mulWadDown(ytBal - ptBal);
            }

Impact

space.balanceOf(this) can be inflated to make user not able to mint shares.

Hacker can use flash loan to manipulate the token balance to mint a large amount of share, then redeem token, and repay the flashloan, profitting at user's fund.

Code Snippet

https://github.com/sherlock-audit/2022-11-sense/blob/main/contracts/src/AutoRoller.sol#L387-L416

Tool used

Manual Review

Recommendation

We recommend use TWAP price and snapshot the user's balance to make the code less vulnerable to flashloan manipulation.

duplicate of #33

[jparklev]

This issue has a decent amount of overlap with #33, so for the previewedLPBal.mulDivDown(totalSupply, _space.balanceOf(address(this))) part of this one, our comment there also represents our thinking here.

For the second part, while it is true that total assets can be adjusted up, that function is cosmetic only and does not put any user funds at risk. From the 4626 standard:

Hacker can use flash loan to manipulate the token balance to mint a large amount of share, then redeem token, and repay the flashloan, profitting at user's fund.

This sounds concerning and we'd love to get to the bottom of it, but unfortunately we don't follow the logic exactly. Similar to the other ticket, if more details or a test case could be provided, it would be enormously helpful.

[aktech297]

I am viewing this as two part.
Part1: - Price manipulation. This has fix.
The fix ensures that the minimum share should be greater than 100 (require(shares > 100);).
I can view difference between ERC4626 and the current implementation. While ERC4626 expect shares > 0, where as the fix expects at least 100 share need to be there. imo, it is putting cap on number of shares need to be there.
do we need this difference ?

Part 2: Flash loan attack.
Need more clarification or POC to decide on this.

[Evert0x]

Labeling as duplicate as both issues rely on the same core issue.