howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/ab18e1f6c03e118158369527baa2487b2b4616b1/src/Vault.sol#L78-L88

Vulnerability details

Impact

Firstly, note that this has been stated in the readMe: https://github.com/code-423n4/2024-07-karak?tab=readme-ov-file#erc20-token-behaviors-in-scope

### ERC20 token behaviors in scope

| Question                                                                                                                                                   | Answer |
| ---------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ |
| [Balance changes outside of transfers](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#balance-modifications-outside-of-transfers-rebasingairdrops) | Yes    |

The protocol aims to work with all sorts of ERC20 tokens including tokens that charge a fee on transfer including PAXG. Deposits and withdrawals of these tokens when being used as staking tokens in vault.sol are not properly handled which can cause a number of accounting issues. The same effect, although to a less degree can be observed with tokens like stETH which has the 1 wei corner case issue in which the token amount transferred can be a bit less than the amount entered in. This also includes other rebasing tokens, and tokens with variable balances or airdrops as the direction of rebasing can lead to the same effect as FOT tokens, which token amount being less than internal accounting suggests, same as extra tokens from positive rebasing and airdrops being lost due it also not matching internal accounting and no other way to claim these tokens.

This means that we should take into account rebasing tokens. Also, the AMPL token, in particular, has a _gonsPerFragment variable that is used to change the balances of other users. The goal of that is to keep a stable price by manipulating the supply.

This creates a huge issue for this particular protocol.

Rebasing tokens can significantly impact the functionality of this Vault contract. The contract is designed as an ERC4626 vault, which assumes a stable relationship between shares and underlying assets. However, rebasing tokens can disrupt this relationship, causing several issues as mentioned above and even more in the POC.

Proof of Concept

Take a look at Deposit Function: https://github.com/code-423n4/2024-07-karak/blob/ab18e1f6c03e118158369527baa2487b2b4616b1/src/Vault.sol#L78-L88

function deposit(uint256 assets, address to)
    public
    override(ERC4626, IVault)
    whenFunctionNotPaused(Constants.PAUSE_VAULT_DEPOSIT)
    nonReentrant
    returns (uint256 shares)
{
    if (assets == 0) revert ZeroAmount();
    return super.deposit(assets, to);
}

This deposit function calls the Solady ERC4626 implementation's deposit function. If the underlying asset is a FOT token, the actual amount received by the vault will be less than the assets parameter due to the transfer fee. However, the contract will mint shares based on the pre-fee amount, leading to an overvaluation of shares.

And then looking at the mint function: https://github.com/code-423n4/2024-07-karak/blob/ab18e1f6c03e118158369527baa2487b2b4616b1/src/Vault.sol#L110-L119

function mint(uint256 shares, address to)
    public
    override(ERC4626, IVault)
    whenFunctionNotPaused(Constants.PAUSE_VAULT_MINT)
    nonReentrant
    returns (uint256 assets)
{
    if (shares == 0) revert ZeroShares();
    assets = super.mint(shares, to);
}

Similar to the deposit function, minting shares for FOT tokens will result in a disparity between the expected asset amount and the actual amount received by the vault.

Also in finishRedeem Function: https://github.com/code-423n4/2024-07-karak/blob/ab18e1f6c03e118158369527baa2487b2b4616b1/src/Vault.sol#L157-L188

function finishRedeem(bytes32 withdrawalKey)
    external
    nonReentrant
    whenFunctionNotPaused(Constants.PAUSE_VAULT_FINISH_REDEEM)
{
    // ... (code omitted for brevity)
    _withdraw({
        by: address(this),
        to: startedWithdrawal.beneficiary,
        owner: address(this),
        assets: redeemableAssets,
        shares: shares
    });
    // ...
}

When withdrawing FOT tokens, the actual amount received by the user will be less than redeemableAssets due to the transfer fee. This leads to users receiving fewer assets than expected.

Also in TotalAssets Function: https://github.com/code-423n4/2024-07-karak/blob/ab18e1f6c03e118158369527baa2487b2b4616b1/src/Vault.sol#L267-L269 The totalAssets function relies on the underlying ERC4626 implementation:

function totalAssets() public view override(ERC4626, IKarakBaseVault) returns (uint256) {
    return super.totalAssets();
}

For rebasing tokens, this function may return an incorrect value as it doesn't account for automatic balance changes due to rebasing events.

Scenarios:

FOT Token Deposit:
- User deposits 1000 tokens with a 1% transfer fee
- Vault receives 990 tokens but mints shares as if it received 1000
- This leads to share overvaluation and potential insolvency
Rebasing Token Balance Change:
- Vault holds 1000 rebasing tokens
- A negative rebase occurs, reducing the balance to 950
- totalAssets still reports 1000, leading to overvaluation of shares
Withdrawal with FOT Token:
- User redeems shares worth 1000 tokens
- Due to the 1% transfer fee, they only receive 990 tokens
- The user incurs an unexpected loss

Tools used

Manual review

Recommended Mitigation Steps

Implement balance checks before and after token transfers to determine the actual amount transferred. Also Use the actual transferred amount for minting shares and updating internal accounting. Most especially, for rebasing tokens, implement a mechanism to periodically update the totalAssets. Additionally, consider adding a "skim" function to handle excess tokens from positive rebases or airdrops and then finally, protocol could consider implementing a whitelist of supported tokens to avoid potential issues with problematic tokens.