Frankencoin - Tricko's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L309-L316

Vulnerability details

Impact

As described in the section below, the restructureCapTable only burns the funds from the first address in addressesToWipe, therefore breaking this method functionality. Despite affecting one of the protocol functionalities, I consider this issue a medium severity one because restructureCapTable will only be needed on the extreme case where reserve is below the minimum equity.

Proof of Concept

As we can see from the code snippet below, during the executing of the for loop, the current variable is set to addressesToWipe[0] in all iterations. Consequently only funds from the address in index zero of addressesToWipe will be burned, all the other ones will remain the same.

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];
        _burn(current, balanceOf(current));
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider modifyig restructureCapTable's code as shown in the diff below.

diff --git a/Equity.orig.sol b/Equity.sol
index 7057ed6..7159cc4 100644
--- a/Equity.orig.sol
+++ b/Equity.sol
@@ -309,9 +309,9 @@ contract Equity is ERC20PermitLight, MathUtil, IReserve {
     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];
+            address current = addressesToWipe[i];
             _burn(current, balanceOf(current));
         }
     }

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L59 https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Equity.sol#L172-L174

Vulnerability details

Impact

The use of block.number as a timing mechanism, as done on Equity contract, can have a significant impact on the reliability of those measurements if Frankencoin were to expand to other chains. While block.number works reasonably well on Ethereum's mainnet, where new blocks are produced at ~12s intervals, it can become problematic on other chains, especially L2 networks, where blocks can be produced much more frequently, potentially every few seconds, and/or have variable rates of production. If Frankencoin is deployed to other chains in the future, this can cause inconsistencies in the timing mechanism, therefore affecting significantly the time-weighted aspect of the votes.

Proof of Concept

For example on two of the biggest L2 networks, Optimism and Arbitrum, each transaction generates a new block, so new blocks are produced at a much faster rate than on mainnet, directly affecting hardcoded contants in the code like uint256 public constant MIN_HOLDING_DURATION = 90*7200. Consequently instead of the expected 90 days of holding time, user's will be able to redeem much earlier.

More importantly, on those networks the rate of block production is not constant, so in periods of high network activity, there will be higher block throughput. So all the votes calculations on the Equity contract will be unreliable. On periods of high or low activity, anchorTime() will increase at different rates, affecting the growth of the time-weighted votes.

Tools Used

Manual Review

Recommended Mitigation Steps

Please consider using block.timestamp instead of block.number, and modify the code and constants accordingly.