Tribe Turbo contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The ERC4626.mint() function doesn't mint the correct amount of tokens. Instead of minting amount number of tokens, it should mint shares number of tokens.

Since the user doesn't receive the correct amount of tokens I'd rate this issue "HIGH".

Proof of Concept

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

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

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

The amount value represents the number of tokens the user has to transfer to the ERC4626 contract while the shares value represents the number of tokens they should receive in return. The current implementation mints amount number of tokens.

In the opposite function, withdraw() you can see the correct logic:

    function redeem(
        uint256 shares,
        address to,
        address from
    ) public virtual returns (uint256 amount) {
        uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals.

        if (msg.sender != from && allowed != type(uint256).max) allowance[from][msg.sender] = allowed - shares;

        // Check for rounding error since we round down in previewRedeem.
        require((amount = previewRedeem(shares)) != 0, "ZERO_ASSETS");

        beforeWithdraw(amount, shares);

        _burn(from, shares);

        emit Withdraw(from, to, amount, shares);

        asset.safeTransfer(to, amount);
    }

The user specifies the amount of tokens they want to redeem and are quoted the number of tokens they will get back.

Or here in the deposit function:

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

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

        _mint(to, shares);

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

User specifies the number of tokens they want to deposit through amount and the function computes the number of tokens that should be minted, shares.

Tools Used

none

Recommended Mitigation Steps

Replace amount with shares:

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

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

        _mint(to, shares);

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

Report

TurboBooster.canSafeBoostVault() has unused parameters

The safe and feiAmount parameters aren't used. The documentation of the function says that the function checks whether the safe is authorized to boost the vault. That's not really the case tho. It just checks whether the boost cap is reached or not. Nothing specific to safe that tries to boost it. I suppose the documentation is just outdated.

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboBooster.sol#L92-L112

testSlurp() fails with specific input

I'm not really sure what the exact issue here is. But, when running the tests the fuzzer found an input combination that caused the testSlurp() test to fail:

uint64 boostAmount = 1;
uint64 donationAmount = 18446744073709551615;
uint256 feePercentage = 1;
address to = 0x00000000000000000000000000000000000298cC;

The following check fails:

assertEq(vault.assetsOf(address(safe)), delta);

Expected: 18446744073709551598
Actual: 0

Here's the test modified with the correct parameter values:

// TurboSafe.t.sol
function testSlurp(
    uint64 boostAmount,
    uint64 donationAmount,
    uint256 feePercentage,
    address to
) public {
    // parameter values that make the test fail
    boostAmount = 1;
    donationAmount = 18446744073709551615;
    feePercentage = 1;
    to = 0x00000000000000000000000000000000000298cC;

    if (boostAmount == 0) boostAmount = 1;

    feePercentage = bound(feePercentage, 0, 1e18);

    safe.deposit(boostAmount, to);

    fei.mint(address(feiCToken), boostAmount);

    booster.setBoostCapForVault(vault, boostAmount);
    booster.setBoostCapForCollateral(asset, boostAmount);

    safe.boost(vault, boostAmount);

    fei.mint(address(vault), donationAmount);

    clerk.setDefaultFeePercentage(feePercentage);

    safe.slurp(vault);

    uint256 protocolFeeAmount = uint256(donationAmount).mulWadDown(feePercentage);

    uint256 safeInterestAmount = donationAmount - protocolFeeAmount;

    uint256 delta = boostAmount + safeInterestAmount;

    assertEq(safe.totalFeiBoosted(), delta);
    assertEq(safe.getTotalFeiBoostedForVault(vault), delta);

    // ----- THIS FAILS
    assertEq(vault.assetsOf(address(safe)), delta);
    // -----

    assertEq(vault.totalAssets(), delta);
    assertEq(feiCToken.borrowBalanceCurrent(address(safe)), boostAmount);

    assertEq(master.totalBoosted(), delta);
    assertEq(master.getTotalBoostedForVault(vault), delta);
    assertEq(master.getTotalBoostedAgainstCollateral(asset), delta);

    assertEq(fei.balanceOf(address(master)), protocolFeeAmount);
}

I tried to figure out where the issue was but didn't manage to find it.

Joey (Joeysantaro) told me that the assetsOf won't be part of the final ERC4626 interface. Since it's still part of the contest tho I think it's right to put this into the report. Also, Joey mentioned that the issue probably stems from the really small fee percentage. A value between 50e16 and 80e16 would be more realistic. The test still fail when you run them with those two values while the other parameters are the same as above.

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/test/TurboSafe.t.sol#L321-L363