Frankencoin - MiloTruck's results

Judge has assessed an item in Issue #928 as 2 risk. The relevant finding follows:

L4

Lines of code

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

Vulnerability details

Bug Description

In the Equity contract, the calculateSharesInternal() function is used to determine the amount of shares minted whenever a user deposits Frankencoin:

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

Note that the return value is the amount of shares minted to the depositor.

Whenever the total amount of shares is less than 1000e18, the depositor will receive 1000e18 - totalShares shares, regardless of how much Frankencoin he has deposited. This functionality exists to mint 1000e18 shares to the first depositor.

However, this is a vulnerability as the total amount of shares can decrease below 1000e18 due to the redeem() function, which burns shares:

Equity.sol#L275-L278

function redeem(address target, uint256 shares) public returns (uint256) {
    require(canRedeem(msg.sender));
    uint256 proceeds = calculateProceeds(shares);
    _burn(msg.sender, shares);

The following check in calculateProceeds() only ensures that totalSupply() is never below 1e18:

Equity.sol#L293

require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share

As such, if the total amount of shares decreases below 1000e18, the next depositor will receive 1000e18 - totalShares shares instead of an amount of shares proportional to the amount of Frankencoin deposited. This could result in a loss or unfair gain of Frankencoin for the depositor.

Impact

If the total amount of shares ever drops below 1000e18, the next depositor will receive a disproportionate amount of shares, resulting in an unfair gain or loss of Frankencoin.

Moreover, by repeatedly redeeming shares, an attacker can force the total share amount remain below 1000e18, causing all future depositors to lose most of their deposited Frankencoin.

Proof of Concept

Consider the following scenario:

• Alice deposits 1000 Frankencoin (amount = 1000e18), gaining 1000e18 shares in return.
• After 90 days, Alice is able to redeem her shares.
• Alice calls redeem() with shares = 1 to redeem 1 share:
  • The total amount of shares is now 1000e18 - 1.
• Bob deposits 1000 Frankencoin (amount = 1000e18). In calculateSharesInternal():
  • totalShares < 1000 * ONE_DEC18 evalutes to true.
  • Bob receives newTotalShares - totalShares = 1000e18 - (1000e18 - 1) = 1 shares.

Although Bob deposited 1000 Frankencoin, he received only 1 share in return. As such, all his deposited Frankencoin can be redeemed by Alice using her shares. Furthermore, Alice can cause the next depositor after Bob to also receive 1 share by redeeming 1 share, causing the total amount of shares to become 1000e18 - 1 again.

Note that the attack described above is possbile as long as an attacker has sufficient shares to decrease the total share amount below 1000e18.

The following Foundry test demonstrates the scenario above:

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

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

contract ShareManipulation_POC is Test {
    Frankencoin zchf;
    Equity reserve;
    
    address ALICE = address(0x1);
    address BOB = address(0x2);

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

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

    function test_NextDepositorGetsOneShare() public {
        // ALICE deposits 1000 Frankencoin, getting 1000e18 shares
        vm.prank(ALICE);
        zchf.transferAndCall(address(reserve), 1000 ether, "");

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

        // ALICE redeems 1 share, leaving 1000e18 - 1 shares remaining
        vm.prank(ALICE);
        reserve.redeem(ALICE, 1);
        
        // BOB deposits 1000 Frankencoin, but gets only 1 share
        vm.prank(BOB);
        zchf.transferAndCall(address(reserve), 1000 ether, "");
        assertEq(reserve.balanceOf(BOB), 1);

        // All of BOB's deposited Frankencoin accrue to ALICE
        vm.startPrank(ALICE);
        reserve.redeem(ALICE, reserve.balanceOf(ALICE) - 1e18);
        assertGt(zchf.balanceOf(ALICE), 1999 ether);
    }
}

Recommendation

As the total amount of shares will never be less than 1e18, check if totalShares is less than 1e18 instead of 1000e18 in 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)));
+         uint256 newTotalShares = totalShares < ONE_DEC18 ? 1000 * ONE_DEC18 : _mulD18(totalShares, _cubicRoot(_divD18(capitalBefore + investment, capitalBefore)));
         return newTotalShares - totalShares;
     }

This would give 1000e18 shares to the initial depositor and ensure that subsequent depositors will never receive a disproportionate amount of shares.

QA Report

Findings Summary

[L-01] Users have no way of revoking their signatures

In ERC20PermitLight.sol, users can use signatures to approve other users as spenders. Spenders will then use the permit() function to verify the signature and gain allowance.

Currently, users have no way of revoking their signatures once it is signed. This could result in a misuse of approvals:

• Alice provides a signature to approve Bob to spend 100 tokens.
• Before deadline is passed, Bob's account is hacked.
• As Alice has no way of revoking the signature, Bob's account uses the signature to approve and spend Alice's tokens.

Recommendation

Implement a function for users to revoke their own signatures, such as a function that increments the nonce of msg.sender when called:

function incrementNonce() public {
    nonces[msg.sender]++;
}

[L-02] MIN_APPLICATION_PERIOD should always be more than 0

In Frankencoin.sol, if MIN_APPLICATION_PERIOD is ever set to 0, anyone can call suggestMinter() with _applicationPeriod = 0, instantly becoming a minter. They will then be able to call sensitive minter functions, such as mint().

Recommendation

Ensure that MIN_APPLICATION_PERIOD is never set to 0. This can be done in the constructor:

Frankencoin.sol#L59-L62

    constructor(uint256 _minApplicationPeriod) ERC20(18){
+      require(_minApplicationPeriod != 0, "MIN_APPLICATION_PERIOD cannot be 0");
       MIN_APPLICATION_PERIOD = _minApplicationPeriod;
       reserve = new Equity(this);
    }

[L-03] Dangerous assumption of stablecoin peg

In StablecoinBridge.sol, users can deposit a chosen stablecoin in exchange for Frankencoin:

StablecoinBridge.sol#L40-L53

/**
* Mint the target amount of Frankencoins, taking the equal amount of source coins from the sender.
* This only works if an allowance for the source coins has been set and the caller has enough of them.
*/
function mint(address target, uint256 amount) public {
    chf.transferFrom(msg.sender, address(this), amount);
    mintInternal(target, amount);
}

function mintInternal(address target, uint256 amount) internal {
    require(block.timestamp <= horizon, "expired");
    require(chf.balanceOf(address(this)) <= limit, "limit");
    zchf.mint(target, amount);
}

Where:

• chf - Address of the input stablecoin contract.
• zchf - Address of Frankencoin contract.

This implementation assumes that the input stablecoin will always have a 1:1 value with Frankencoin. However, in the unlikely situation that the input stablecoin depegs, users can use this stablecoin bridge to mint Frankencoin at a discount, thereby harming the protocol.

Recommendation

Use a price oracle to ensure the input stablecoin price is acceptable. Alternatively, implement some method for a trusted user to intervene, such as allowing a trusted user to pause minting in the event the input stablecoin depegs.

[L-04] Incorrect code in restructureCapTable()

In Equity.sol, the restructureCapTable() function contains an error:

Equity.sol#L309-L316

function restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe) public {
    require(zchf.equity() < MINIMUM_EQUITY);
    checkQualified(msg.sender, helpers);
    for (uint256 i = 0; i<addressesToWipe.length; i++){
        address current = addressesToWipe[0]; // @audit Should be addressesToWipe[i]
        _burn(current, balanceOf(current));
    }
}

[L-05] Minor inconsistency with comment in calculateProceeds()

In Equity.sol, the calculateProceeds() function is as shown:

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

The comment states that a minimum of one share (ONE_DEC18) must remain in the contract. However, the require statement actually ensures that totalShares is never below ONE_DEC18 + 1.

Recommendation

Change the require statement to the following:

require(totalShares - shares >= ONE_DEC18, "too many shares"); // make sure there is always at least one share

[L-06] Users can lose a significant amount of votes by transferring shares to themselves

In Equity.sol, adjustRecipientVoteAnchor() is called by the _beforeTokenTransfer() hook to adjust a recevier's voteAnchor:

Equity.sol#L150-L167

/**
* @notice the vote anchor of the recipient is moved forward such that the number of calculated
* votes does not change despite the higher balance.
* @param to        receiver address
* @param amount    amount to be received
* @return the number of votes lost due to rounding errors
*/
function adjustRecipientVoteAnchor(address to, uint256 amount) internal returns (uint256){
    if (to != address(0x0)) {
        uint256 recipientVotes = votes(to); // for example 21 if 7 shares were held for 3 blocks
        uint256 newbalance = balanceOf(to) + amount; // for example 11 if 4 shares are added
        voteAnchor[to] = uint64(anchorTime() - recipientVotes / newbalance); // new example anchor is only 21 / 11 = 1 block in the past
        return recipientVotes % newbalance; // we have lost 21 % 11 = 10 votes
    } else {
        // optimization for burn, vote anchor of null address does not matter
        return 0;
    }
}

The function is used to ensure a user has the same amount of votes before and after a transfer of shares.

However, if a user transfers all his shares to himself, newBalance will be two times of his actual balance, causing his voteAnchor to become larger than intended. Therefore, he will lose a significant amount of votes.

Recommendation

Make the following change to the _beforeTokenTransfer() hook:

Equity.sol#L112-L114

     function _beforeTokenTransfer(address from, address to, uint256 amount) override internal {
         super._beforeTokenTransfer(from, to, amount);
-        if (amount > 0){
+        if (amount > 0 && from != to){

This ensures users do not lose votes in the unlikely event where they transfer shares to themselves.

[L-07] One share will always be stuck in the Equity contract

In Equity.sol, the following require statement ensures that the last share can never be redeemed from the contract:

Equity.sol#L293

require(shares + ONE_DEC18 < totalShares, "too many shares"); // make sure there is always at least one share

In the unlikely event where everyone wants to redeem their shares, the last redeemer will never be able to redeem his last share. Therefore, he will always have some Frankencoin stuck in the contract.

[L-08] Misleading implementation of vote delegation

In Equity.sol, users can delegate their votes to a delegatee using the delegateVoteTo() function.

The canVoteFor() function is then used to check if delegate is a delegatee of owner:

Equity.sol#L225-L233

function canVoteFor(address delegate, address owner) internal view returns (bool) {
    if (owner == delegate){
        return true;
    } else if (owner == address(0x0)){
        return false;
    } else {
        return canVoteFor(delegate, delegates[owner]);
    }
}

However, due to its recursive implementation, delegations can be chained to combined the votes of multiple users, similar to a linked list. For example:

• Alice's delegatee is Bob.
• Bob's delegatee is Charlie.

Due to the chained delegation, Charlie's voting power is increased by the votes of both Alice and Bob.

However, this becomes an issue if Alice wishes to delegate votes to only Bob, and no one else. Once she sets Bob as her delegatee, Bob's delegatee will also gain voting power from her votes; Alice has no control over this.

Recommendation

Consider removing this chained delegation functionality in canVoteFor():

function canVoteFor(address delegate, address owner) internal view returns (bool) {
    return owner == delegate;
}

[N-01] _transfer() doesn't check that sender != address(0)

In ERC20.sol, the _transfer() function does not ensure that the sender is not address(0):

ERC20.sol#L151-L152

function _transfer(address sender, address recipient, uint256 amount) internal virtual {
    require(recipient != address(0));

This differs from OpenZeppelin's ERC20 implementation.

Recommendation

Add the following check to _transfer():

ERC20.sol#L151-L152

     function _transfer(address sender, address recipient, uint256 amount) internal virtual {
+        require(sender != address(0));
         require(recipient != address(0));

[N-02] Correctness of _cubicRoot()

The following Foundry fuzz test can be used to verify the correctness of the _cubicRoot() function:

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

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

contract MathUtilTest is Test, MathUtil {
    function testFuzz_cubicRoot(uint256 v) public {
        v = bound(v, ONE_DEC18, 38597363079105398489064286242003304715461606497555392182630);
        uint256 result = _power3(_cubicRoot(v));
        if (result != v) {
            uint256 err = v > result ? v - result : result - v;
            assertLt(err * 1e8, v);
        } 
    }
}

The test proves that _cubicRoot() has a margin of error below 0.000001% when _v >= 1e18`.

Additionally, the function has the following limitations:

• _cubicRoot() reverts if _v is greater than 38597363079105398489064286242003304715461606497555392182630.
• _cubicRoot() is incorrect for extremely small values of _v. For example:
  • _cubicRoot(0) returns 7812500000000000.
  • _cubicRoot(1) returns 7812500000003509.

Nevertheless, these limitations do not pose any issue in the current implementation of the protocol.