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

1 stars 0 forks source link

QA Report #39

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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

GalloDaSballo commented 2 years ago

Great formatting, and thorough report. Unfortunately only 1 and a half valid findings.

Would recommend the warden to keep at it and ideally find more, the presentation is there just needs more substance!

GalloDaSballo commented 2 years ago

4/10 extra point for the extra work, ultimately finding is a lot more relevant than a casual "no check eheh XD"