Tribe Turbo contest - CertoraInc's results

Lines of code

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

Vulnerability details

bug in the mint function of the ERC4626 contract

The mint function recieves an amount of shares and an address to and mints the amount of shares to the to address. The sender must transfer an amount of token, so that the ratio will be saved - shares / totalShares = amountOfToken / totalAssets(), where the totalAssets() is a virtual function that will be implemented in the sub contract. After the sender sends the tokens, it mints the shares. In the given implementation of ERC4626 the mint function mints the calculated amount of token instead of the given shares amount. This can be seen in the following code (link is also attached):

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);
}

We can see the call _mint(to, amount) with the amount parameter, which is the amount of token the user transferred, instead of the shares parameter, which is the given shares that supposed to be minted.

Impact

The impact of this bug can be really big. Let's say that the vault currently has token amount (i.e. totalAssets()) = x and shares amount (i.e. totalSupply) = s.

In a normal case, when a user wanted to mint a shares, he had to transfer ax/s tokens in order to maintain the ratio. After it, if the user wanted to get his tokens back, he need to call redeem(a), which burns a shares and transfer a*(x+ax/s)/(s+a)=ax/s tokens to the user. After these 2 actions that cancel each other, the system supposed to return to the state it started from - token amount (i.e. totalAssets()) = x and shares amount (i.e. totalSupply) = s.

But this is not the case - because of the bug in the mint function, when a user wants to mint a shares, he will transfer ax/s tokens, but the fucntion will mint ax/s shares to him instead of a shares. So when the user will call redeem(a) to redeem his shares and get his tokens back if x < s the user won't be able to get his tokens back and he'll lose tokens - he'll be able to redeem ax/s shares, which will give him a * (x + ax/s) / (s + ax/s) = ax/s * (s+a)/(s**2/x + a), which is less than ax/s because x < s. But that's not even the most interesting part. If x > s, like before, the user wants to mint a shares and he will transfer ax/s tokens, but the fucntion will mint ax/s shares to him instead of a shares. In this case, the user gets more shares than he should. That means that the user can get back more token than he gave. The user transferred ax/s tokens and got ax/s shares, but when he'll call redeem with all of his shares he'll recieve a * (x + ax/s) / (s + ax/s) = ax/s * (s+a)/(s**2/x + a), which is more than ax/s tokens because x > s.

So, if indeed x > s, an attacker can exploit this bug, and for example take a flash loan of the token, mint shares and redeem them and earn more tokens (and of course return the flash loan).

links

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

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboRouter.sol#L49 https://github.com/fei-protocol/ERC4626/blob/5b786fe0317f65f5b716f577c28092fa349c4903/src/ERC4626RouterBase.sol#L29 https://github.com/Rari-Capital/solmate/blob/main/src/mixins/ERC4626.sol#L47

Vulnerability details

Impact

Both createSafeAndDeposit function and createSafeAndDepositAndBoost would revert on every call.

Proof of Concept

Both functions suffers from the same mistake so I'll detailed only on createSafeAndDeposit (link 1). First the function calls master.createSafe(underlying); to create a safe. the msg.sender in the master.createSafe() contex is the TurboRouter contract, so the owner of the safe that was created is the TurboRouter contract. Now we call super.deposit(IERC4626(address(safe)), to, amount, minSharesOut); (implemention of this deposit funciton is in link 2). In there we can see the next line: if ((sharesOut = vault.deposit(amount, to)) < minSharesOut), let's jump to the implementation of vault.deposit(amount, to)) (link 3). The msg.sender in this contex is again the TurboRouter contract. Which means in this line: asset.safeTransferFrom(msg.sender, address(this), amount); we trasfer from the TurboRouter contract to the safe, so the TurboRouter contract has to give allowence for the safe contract, but it does not happen anywhere, so the trasferFrom call will revert.

Tools Used

read the code

Recommended Mitigation Steps

Consider give approvemence from the TurboRouter contract to the created safe.

Low and non-critical bugs

wrong implementation of ERC4626RouterBase's withdraw function

In the interface we can see the function description:

/** 
 @notice withdraw `amount` from an ERC4626 vault.
 @param vault The ERC4626 vault to withdraw assets from.
 @param to The destination of assets.
 @param amount The amount of assets to withdraw from vault.
 @param minSharesOut The min amount of shares received by `to`.
 @return sharesOut the amount of shares received by `to`.
 @dev throws MinOutError   
*/
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 minSharesOut
) external payable returns (uint256 sharesOut);

As you probably know, the ERC4626 withdraw function recieves 3 parameters - from, to and amount. It withdraws amount tokens from the vault and transfer them to to, and burns the calculated amount of shares from from. So we can see that the minSharesOut is really wrong to use, because it actually burns the shares from from and not giving the shares to to. The correct implementation should use maxSharesIn, in order to specify the maximum amount of shares the user is willing to burn. It should look something like this:

/// @inheritdoc IERC4626RouterBase
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesIn
) public payable virtual override returns (uint256 sharesIn) {
    if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
        revert MinAmountError();
    }
}

As far as I know, this bug can't be exploited, because it only limits the users' functionallity, but it is a bug that needs to be fixed. In addition to limiting the users' functionallity, I think that users will prefer the maxSharesOut fix that I suggested, to limit the amount of shares that wil be burned and avoiding burning to much shares to get a small amount of tokens.

Confusing error messages

Another thing that is half related to the previous bug is the MinAmountError. It is correct in the redeem function, but in the other function I think that the errors should be more specific. I think that this is a more understood implementation (using the alternative withdraw function I suggested before):

/// @inheritdoc IERC4626RouterBase
function mint(
    IERC4626 vault, 
    address to,
    uint256 shares,
    uint256 maxAmountIn
) public payable virtual override returns (uint256 amountIn) {
    if ((amountIn = vault.mint(shares, to)) > maxAmountIn) {
        revert MaxAmountError();
    }
}
/// @inheritdoc IERC4626RouterBase
function deposit(
    IERC4626 vault, 
    address to,
    uint256 amount,
    uint256 minSharesOut
) public payable virtual override returns (uint256 sharesOut) {
    if ((sharesOut = vault.deposit(amount, to)) < minSharesOut) {
        revert MinSharesError();
    }
}
/// @inheritdoc IERC4626RouterBase
function withdraw(
    IERC4626 vault,
    address to,
    uint256 amount,
    uint256 maxSharesIn
) public payable virtual override returns (uint256 sharesIn) {
    if ((sharesIn = vault.withdraw(amount, to, msg.sender)) > maxSharesIn) {
        revert MaxSharesError();
    }
}
/// @inheritdoc IERC4626RouterBase
function redeem(
    IERC4626 vault,
    address to,
    uint256 shares,
    uint256 minAmountOut
) public payable virtual override returns (uint256 amountOut) {
    if ((amountOut = vault.redeem(shares, to, msg.sender)) < minAmountOut) {
        revert MinAmountError();
    }
}

The change is that the error that is reverting gives more information about the error happened. Reverting with a MinAmountError in every function can be very confusing.

Gas Optimizations

inline some functions to save gas

In the ERC4626, there are multiple functions that their code can be inlined to save the gas spending on calling them.

function previewDeposit(uint256 amount) public view virtual returns (uint256) {
    uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
    return supply == 0 ? amount : amount.mulDivDown(supply, totalAssets());
}
function previewMint(uint256 shares) public view virtual returns (uint256) {
    uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
    return supply == 0 ? shares : shares.mulDivUp(totalAssets(), supply);
}
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());
}
function previewRedeem(uint256 shares) public view virtual returns (uint256) {
    uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.
    return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
}

For example, the previewDeposit function. It is used 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");
    // ...
}

Instead of calling that function, shares can be calculated by inlining the function, something like this:

function deposit(uint256 amount, address to) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
    shares = totalSupply; // saving totalSupply to save a SLOAD, and using shares variable to avoid creating a new variable.
    shares = (shares == 0 ? amount : amount.mulDivDown(shares, totalAssets()))
    require(shares != 0, "ZERO_SHARES");
    // ...
}

This can be sone to all of the other functions I attached before (previewMint, previewWithdraw, previewRedeem).

Save mapping values instead of accessing it twice

Save the getTotalFeiBoostedForVault[vault] in the slurp function of the TurboSafe instead of accessing it twice for reading. It will look something like this:

function slurp(ERC4626 vault) external nonReentrant requiresLocalOrMasterAuth {
    uint256 totalFeiBoostedForVault = getTotalFeiBoostedForVault[vault];
    require(totalFeiBoostedForVault != 0, "NO_FEI_BOOSTED");
    
    // ...

    unchecked {
        // Update the total Fei held in the Vault proportionately.
        // Cannot overflow because the total cannot be less than a single Vault.
        getTotalFeiBoostedForVault[vault] = totalFeiBoostedForVault + safeInterestAmount;
    }
    // ...
}