search

code-423n4 / 2024-05-bakerfi-findings

4 stars 4 forks source link

Incorrect formula to compute tokenPerETH rate renders unnusable the max deposit feature. #40

Closed c4-bot-4 closed 5 months ago

c4-bot-4 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-05-bakerfi/blob/main/contracts/core/Vault.sol#L339

Vulnerability details

Impact

The tokenPerETH rate is computed incorrectly, which will cause problems with the max deposit feature. (Will provide some examples of the calculations in the Proof of Concept section)

Proof of Concept

When making a deposit in a Vault, if the maxDeposit feature is enabled, the execution will validate if the depositor should be allowed to deposit the requested amount. To determine if the depositor should be allowed or not, the function will compute the afterDeposit value of the user deposits including the deposit being made.

function deposit(
    address receiver
)
    ...
{
    ...

    // Verify if the Deposit Value exceeds the maximum per wallet
    uint256 maxDeposit = settings().getMaxDepositInETH();
    if (maxDeposit > 0) {
        uint256 afterDeposit = msg.value +
        //@audit-info => Current existing deposited amount of the depositor
            ((balanceOf(msg.sender) * _tokenPerETH(maxPriceAge)) / 1e18);
        if (afterDeposit > maxDeposit) revert MaxDepositReached();
    }

    ...
}

//@audit-info => Computes the amount of ETH that each share can claim!
function _tokenPerETH(uint256 priceMaxAge) internal view returns (uint256) {
    uint256 position = _totalAssets(priceMaxAge);
    if (totalSupply() == 0 || position == 0) {
        return 1 ether;
    }

    //@audit-issue => Incorrect formula causes the tokenPerETH rate to be computed wrong
    return (totalSupply() * 1 ether) / position;
}

There is a problem in the formula to determine the current tokenPerETH rate. The formula that is used to determine the rate is incorrectly computing the rate, the current formula is (totalSupply() * 1 ether) / position;. By doing some math using the current formula we can see the problem with it. Example 1: totalAssets > totalSupply

Example 2: totalSupply > totalAssets

As we've just seen, the current formula is incorrectly computing the tokenPerETH rate. I coded a fuzz test using foundry to prove my statements. Please, help me to install foundry on the project at the root directory, and set the total runs in the foundry.toml to 1000. As an example, this is how my foundry.toml file looks like

[profile.default]
src = "src"
out = "artifacts"
libs = ["node_modules", "lib"]
remappings = [
    "@aave/=node_modules/@aave/",
    "@openzeppelin/=node_modules/@openzeppelin/",
    "eth-gas-reporter/=node_modules/eth-gas-reporter/",
    "hardhat/=node_modules/hardhat/",
]

[fuzz]
runs = 1000

Coded PoC

Once you have added foundry to the project, help me to add the below test in a new file.

In the PoC we are fuzzing the tokenPerETH() by passing random values for the totalAssets and totalSupply, this would help us to test multiple combinations of values for the 2 variables, which would represent the state of the vault at different points in time.

Help me to run the PoC for the first time without doing any modifications, this first execution uses the current formula and we are expecting to see the fuzz test to fail because the formula being wrong.

For the second execution, help me to comment the current formula and uncomment the correct formula inside the _tokenPerETH() function of the PoC. This time we expect to see the fuzz test to pass all the runs.

The fuzz test basically determines the tokenPerETH rate and it multiplies it by the totalSupply (totalShares), and the expected result is to be exactly equals to the original totalAssets.

Command to run the test: forge test --match-test test_tokenPerETH_PoC -vvvv

Expand to see PoC
``` // SPDX-License-Identifier: Apache-2.0 pragma solidity ^0.8.0; import "forge-std/Test.sol"; contract TestVault is Test { function _tokenPerETH(uint256 position, uint256 totalSupply) internal view returns (uint256) { //@audit-info => Current implementation // uint256 position = _totalAssets(priceMaxAge); // if (totalSupply() == 0 || position == 0) { // return 1 ether; // } // return (totalSupply() * 1 ether) / position; //@audit => Run the fuze test the first time with the original formula uncommented to verify the formula is wrong and causes the test to fail //@audit-info => Current formula! return (totalSupply * 1 ether) / position; //@audit => Run the fuze test the second time with the original formula commented and the below formula uncommented to verify the formula is wrong and causes the test to fail //@audit-info => Correct formula // return (position * 1 ether) / totalSupply; } // forge test --match-test test_tokenPerETH_PoC -vvvv function test_tokenPerETH_PoC(uint256 totalAssets, uint256 totalSupply) external { uint256 totalAssets = bound(totalAssets, 0.001 ether, 100000e18); uint256 totalSupply = bound(totalSupply, 0.001 ether, 100000e18); uint256 tokenPerEthRate = _tokenPerETH(totalAssets, totalSupply); //@audit-info => Assert that the tokenPerEthRate multiplied by the total supply and divide by 1e18 is aproximately equals (rounding difference) to the totalAssets! assertApproxEqAbs(totalAssets, (totalSupply * tokenPerEthRate) / 1e18, 1e9); } } ```

Tools Used

Manual Audit & Foundry

Recommended Mitigation Steps

Update the formula that computes the tokenPerETH rate to return (position * 1 ether) / totalSupply();

function _tokenPerETH(uint256 priceMaxAge) internal view returns (uint256) {
    uint256 position = _totalAssets(priceMaxAge);
    if (totalSupply() == 0 || position == 0) {
        return 1 ether;
    }
-   return (totalSupply() * 1 ether) / position;
+   return (position * 1 ether) / totalSupply();
}

Assessed type

Math

c4-judge commented 5 months ago

0xleastwood marked the issue as primary issue

c4-judge commented 5 months ago

0xleastwood marked the issue as duplicate of #29

c4-judge commented 5 months ago

0xleastwood marked the issue as satisfactory