code423n4 commented 2 years ago

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).