code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L151-L155

Vulnerability details

The TurboSafe's totalAssets() function is used by ERC4626.previewDeposit(), ERC4626.previewMint(), ERC4626.previewWithdraw(), and ERC4626.previewRedeem(). These preview functions are called directly by the non-preview versions and therefore if totalAssets() has the wrong value, all other calculations will be wrong.

Impact

totalAssets() can be made to overflow under extraordinary circumstances, causing the misspricing of assets.

Proof of Concept

    /// @notice Returns the total amount of assets held in the Safe.
    /// @return The total amount of assets held in the Safe.
    function totalAssets() public view override returns (uint256) {
        return assetTurboCToken.balanceOf(address(this)).mulWadDown(assetTurboCToken.exchangeRateStored());
    }

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboSafe.sol#L151-L155

totalAssets() involves a call to mulWadDown which uses assembly which does not have overflow protections

    function mulWadDown(uint256 x, uint256 y) internal pure returns (uint256) {
        return mulDivDown(x, y, WAD); // Equivalent to (x * y) / WAD rounded down.
    }

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/utils/FixedPointMathLib.sol#L14-L16

    function mulDivDown(
        uint256 x,
        uint256 y,
        uint256 denominator
    ) internal pure returns (uint256 z) {
        assembly {
            // Store x * y in z for now.
            z := mul(x, y)

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/utils/FixedPointMathLib.sol#L34-L41

    function previewWithdraw(uint256 amount) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? amount : amount.mulDivUp(supply, totalAssets());
    }

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L143-L147

    function withdraw(
        uint256 amount,
        address to,
        address from
    ) public virtual returns (uint256 shares) {
        shares = previewWithdraw(amount); // No need to check for rounding error, previewWithdraw rounds up.

https://github.com/Rari-Capital/solmate/blob/1205a9067ff957ef8b0b003ff9d77c20ef9f2e0b/src/mixins/ERC4626.sol#L74-L79

The video linked to in the README.md talks about the fact that a safe can be owned by a DAO with lots of depositors and therefore it's possible for a large amount of assets to be added to the safe. If this happens along with a very large exchange rate the z := mul(x, y) assembly call can overflow.

Tools Used

Code inspection

Recommended Mitigation Steps

Use normal multiplication and division, and provide a way to withdraw funds in the case where totalAssets() reverts.

transmissions11 commented 2 years ago

overflow is checked further down in mulDiv, false positive

GalloDaSballo commented 2 years ago

Agree with the sponsor, there is an overflow check on this line: https://github.com/Rari-Capital/solmate/blob/0a121c54b23ccde77bb0dd84a59b71783c126894/src/utils/FixedPointMathLib.sol#L44

code-423n4 / 2022-02-tribe-turbo-findings