Share accounting is incorrect

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L266-L270 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L290-L297

Vulnerability details

Bug Description

Share Distrbution

In the Equity contract, the amount of shares minted to a depositor is determined using calculateSharesInternal():

Equity.sol#L266-L270

function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {
    uint256 totalShares = totalSupply();
    uint256 newTotalShares = totalShares < 1000 * ONE_DEC18 ? 1000 * ONE_DEC18 : _mulD18(totalShares, _cubicRoot(_divD18(capitalBefore + investment, capitalBefore)));
    return newTotalShares - totalShares;
}

Where:

• capitalBefore - Total amount of Frankencoin in reserves before the deposit.
• investment - Amount of Frankencoin deposited by the user.

The amount of shares minted can be simplified into the following formula:

$$ shares = totalShares \times \biggl( \sqrt[3]{ 1 + \frac{investment}{capitalBefore} } - 1 \biggr) $$

Due to the cube root, users will receive less shares for depositing the same amount of Frankencoin when capitalBefore is larger. Assume the following:

• totalShares = 1000
• capitalBefore = 1000

If a user deposits 1000 Frankencoin, he gains the following amount of shares:

$$ shares = 1000 \times \biggl( \sqrt[3]{ 1 + \frac{1000}{1000} } - 1 \biggr) \approx 260 $$

Now, if another user deposits 1000 Frankencoin, he will get less shares:

$$ shares = 1260 \times \biggl( \sqrt[3]{ 1 + \frac{1000}{2000} } - 1 \biggr) \approx 182 $$

Share Redemption

Similarly, the calculateProceeds() function is used to calculate how much Frankencoin a user should get for burning shares:

Equity.sol#L290-L297

function calculateProceeds(uint256 shares) public view returns (uint256) {
    uint256 totalShares = totalSupply();
    uint256 capital = zchf.equity();
    require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share
    uint256 newTotalShares = totalShares - shares;
    uint256 newCapital = _mulD18(capital, _power3(_divD18(newTotalShares, totalShares)));
    return capital - newCapital;
}

Where:

• shares - Amount of shares burned by the user.

The amount of Frankencoin can be simplified into the formula below:

$$ proceeds = capital \times \biggl( 1 - \biggl( 1 - \frac{shares}{totalShares} \biggr)^3 \biggr)$$

Due to the power 3, users will receive less Frankencoin for burning the same amount of shares when totalShares is smaller. Assume the following:

• capital = 1000
• totalShares = 1000

If a user redeems 100 shares, he gains the following amount of Frankencoin:

$$ proceeds = 1000 \times \biggl( 1 - \biggl( 1 - \frac{100}{1000} \biggr)^3 \biggr) = 271 $$

Now, if another user redeems 100 shares, he will get less Frankencoin:

$$ proceeds = 729 \times \biggl( 1 - \biggl( 1 - \frac{100}{900} \biggr)^3 \biggr) = 217 $$

Impact

When the total amount of Frankencoin in the Equity contract is large, depositors will gain less shares, thereby losing a portion of their assets. Likewise, when the total amount of shares is small, users that redeem shares will get less Frankencoin in return.

As a user's amount of shares corresponds to his voting power, this also affects all voting functionality in the protocol.

Proof of Concept

The following code contains two tests:

• test_ShareDistributionIsWrong() demonstrates how the distribution of shares is incorrect.
• test_RedemptionIsWrong() illustrates how the amount of Frankencoin gained from redemptions is incorrect.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "../contracts/Frankencoin.sol";

contract ShareMath_POC is Test {
    Frankencoin zchf;
    Equity reserve;

    address ALICE = address(0x1);
    address BOB = address(0x2);
    address CHARLIE = address(0x3);

    function setUp() public {
        // Setup contracts
        zchf = new Frankencoin(10 days);
        reserve = Equity(address(zchf.reserve()));

        // Give users 1000 Frankencoin each
        zchf.suggestMinter(address(this), 0, 0, "");
        zchf.mint(ALICE, 1000 ether);
        zchf.mint(BOB, 1000 ether);
        zchf.mint(CHARLIE, 1000 ether);
    }

    function test_ShareDistributionIsWrong() public {
        // All users deposit 1000 Frankencoin each
        vm.prank(ALICE);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        vm.prank(BOB);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        vm.prank(CHARLIE);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        // Everyone gets a different amount of shares (later depositors get less)
        assertEq(reserve.balanceOf(ALICE), 1000e18);
        assertEq(reserve.balanceOf(BOB), 259920634920634920000);
        assertEq(reserve.balanceOf(CHARLIE), 182328456244376202189);
    }

    function test_RedemptionIsWrong() public {
        // All users deposit 1000 Frankencoin each
        vm.prank(ALICE);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        vm.prank(BOB);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        vm.prank(CHARLIE);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

        // Time passes until they can redeem
        vm.roll(block.number + 90 * 7200);

        // Each user redeems 100 shares each
        vm.prank(ALICE);
        reserve.redeem(ALICE, 100 ether);

        vm.prank(BOB);
        reserve.redeem(BOB, 100 ether);

        vm.prank(CHARLIE);
        reserve.redeem(CHARLIE, 100 ether);

        // Everyone gets a different amount of Frankencoin (later redeemers get less)
        assertEq(zchf.balanceOf(ALICE), 581757839367391683000);
        assertEq(zchf.balanceOf(BOB), 501222813631772924205);
        assertEq(zchf.balanceOf(CHARLIE), 426687793876096230725);
    }
}

Recommendation

Consider simplifying both formulas:

• Calculate the amount of shares to mint as totalShares * investment / capitalBefore:

function calculateSharesInternal(uint256 capitalBefore, uint256 investment) internal view returns (uint256) {
    uint256 totalShares = totalSupply();
    if (totalShares < 1000 * ONE_DEC18) {
        return 1000 * ONE_DEC18 - totalShares;
    }
    return _mulD18(totalShares, _divD18(investment, capitalBefore));
}

• Calculate the amount of Frankencoin redeemed as capital * shares / totalShares:

function calculateProceeds(uint256 shares) public view returns (uint256) {
    require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share
    return _mulD18(zchf.equity(), _divD18(shares, totalSupply()));
}

Alternatively, use an existing implementation of ERC4626 to handle share accounting, such as Openzeppelin or solmate.

[c4-pre-sort]

0xA5DF marked the issue as primary issue

[0xA5DF]

Might be the intended design of the protocol, will leave open for sponsor to comment

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #744

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid