Frankencoin - Udsen's results

Lines of code

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

Vulnerability details

Impact

In the Equity.restructureCapTable() function, the address[] of addressesToWipe is used to burn the FPS tokens of the passive FPS holders when the system is bootstrap by the qualified FPS holders. In the for loop each of the addressesToWipe are assigned to address current and then burnt as follows:

    for (uint256 i = 0; i<addressesToWipe.length; i++){
        address current = addressesToWipe[0]; 
        _burn(current, balanceOf(current));
    }

The problems is address current is always assigned addressesToWipe[0]. which means only the FPS tokens of the addressesToWipe[0] will be burnt during the iterations of the for loop. Hence the shares of the other addresses will remain intact. So the qualified brave souls will share other one million with the existing passive FPS holders without owning 100% of the all FPS shares.

Hence this breaks a core functionality of the protocol. And the qualified brave FPS holders will lose their provided equity since it can be retrieved as profit by the existing passive FPS holders by redeeming their shares. This is possible due to passive FPS holders are not wiped out due to the error in the above depicted code. The qualified brave holders will lose their equity and existing passive FPS holders will gain profit unfairly as a result.

Proof of Concept

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

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

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

Above for loop should be corrected as below:

    for (uint256 i = 0; i<addressesToWipe.length; i++){
        address current = addressesToWipe[i]; 
        _burn(current, balanceOf(current));
    }

Lines of code

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

Vulnerability details

Impact

In the Equity.sol protocol the MIN_HOLDING_DURATION is used in the Equity.canRedeem() function, to check whether the given address is allowed to redeem FPS. The redeem is possible if the average holding duration of the given address is greater than or equal to MIN_HOLDING_DURATION.

Currently the MIN_HOLDING_DURATION is set to be 90 days and given in blocks as 90*7200. Hence as it is seen the blocks per day is hardcoded here to be 7200 blocks.

Hence if the block time changes in the future the MIN_HOLDING_DURATION of 90 days would change.

This will create discrepancy in the locked period of the pool shares of the protocol. People will lose trust since at times they will not be able to redeem their shares even after 90 days have passed by, and on other times they can redeem the FPS even before the 90 days period has passed.

Hence this breaks a core functionality of the protocol.

Proof of Concept

Average block time of ethereum can change overtime. Hence the blocks per day can also change.

- Eg : If block time is 15seconds - then blocks per day = 5760 
       The number of days = 90*7200 / 5760 = 112.5 days.
       Locked period is no longer 90 days and now has increased to 112.5 days

- Eg : If block time is 10 seconds - then blocks per day = 8640
       The number of days = 90*7200 / 8640 = 75 days.
       Locked period is no longer 90 days and now has decreased to 75 days

Following line depicts the code line which hardcodes the number of blocks per day:

    uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

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

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

Rather than declaring MIN_HOLDING_DURATION as a constant better to configure MIN_HOLDING_DURATION as an updatable value. Hence it can be changed accordingly, when the block time changes in the future.

1. TRANSACTION WILL REVERT IF THERE IS ANY NON-DELEGATED ADDRESS IN THE helpers address[] ARRAY, WHEN TRYING TO CALCULATE THE votes OF THE DELEGATE

Comment in the Equity.checkQualified() states:

It is the responsiblity of the caller to figure out whether helpes are necessary and to identify them by scanning the blockchain for Delegation events

But in the implementation of the Equity.votes(address, address[]) function, it calls the Equity.canVoteFor() function, which makes sure if a helper has not delegated to the owner directly or indirectly the transaction will revert.

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

Since the helpers should be found manually by the user, it is prone to error, and there is a high possiblity that an user will include a non-delegated helper in the helper[] array, which will revert the entire transaction thus wasting gas.

Hence it is recommended to use if-else structure instead of require and calculate the votes of only the delegated helpers in the helpers address[] array and ignore the non-delegated addresses if any, without revert. Remove the following require statement in the Equity.votes() function and replace it with a if-else statement.

require(canVoteFor(sender, current));

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L195 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L225-L233

2. < SHOULD BE REPLACED BY <= TO MAKE SURE THERE IS ATLEAST ONE SHARE IN THE require STATEMENT OF THE Equity.calculateProceeds() FUNCTION.

In Equity.calculateProceeds() function there is a require statement to check the number of ZCHF tokens to redeem.

require(shares + ONE_DEC18 < totalShares, "too many shares")

The comment here states: make sure there is always at least one share.

According to the above require statement it reverts when there is atleast one share. And the require statement will only hold when there is atleast two shares.

Eg: let's assume totalShares = 1000 and shares = 999, so there is atleast one share.

But the require(999 + 1 < 1000) will be false and the transaction will revert. Hence above comment will not stand.

Hence the require statement should be updated as below:

require(shares + ONE_DEC18 <= totalShares, "too many shares")  

In this case it will make sure there is at least one share.

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

3. Equity.redeem TARGET ADDRESS SHOULD BE CHECKED FOR address(0)

In Equity.redeem function, when redeeming the pool shares for ZCHF tokens, the transfer function is called as follows:

zchf.transfer(target, proceeds);

But the target is never checked for address(0). If address target is set to address(0) erroneously, the proceeds could be sent to address(0) and burnt .

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

4. ADDRESS sender SHOULD BE CHECKED FOR address(0) IN THE equity.votes FUNCTION.

Equity.votes(address, address[]) and Equity.checkQualified() are public functions. Hence if address(0) is passed in as sender address to these functions, the Equity.canVoteFor() will always return true when (owner == delegate) is checked and both addresses are address(0).

function votes(address sender, address[] calldata helpers) public view returns (uint256) {
    uint256 _votes = votes(sender);
    for (uint i=0; i<helpers.length; i++){
        address current = helpers[i];
        require(current != sender);
        require(canVoteFor(sender, current));
		...
		}
}

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

Hence this will erroneously caclulate the votes inside the Equity.votes() function when the function actually should revert instead. - Equity.sol#L193

Current use case of this function is in Equity.restructureCapTable() which calls the checkQualified() function as below:

        checkQualified(msg.sender, helpers);

Since the msg.sender will not equal to address(0), there is no direct threat. But since these are public functions and future changes to the contract could use them for different logic implementations it is recommended to check for the address(0)

Or else if and else-if should interchange in Equity.canVoteFor(), so that address(0) check will be performed prior to the owner == delegate check. Hence if owner == address(0) is checked first then the Equity.canVoteFor() will return false

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L193 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L226-L229

5. IN Frankencoin.constructor THE _minApplicationPeriod SHOULD BE CHECKED FOR 0 VALUE (INPUT VALIDATION).

In Frankencoin.constructor() function, the immutable variable MIN_APPLICATION_PERIOD is set as follows:

  MIN_APPLICATION_PERIOD = _minApplicationPeriod;	
  

But the _minApplicationPeriod is never checked for 0 value. It is recommended to perform input validation since it is being assigned to an immutable variable and can not be changed after assignment.

_minApplicationPeriod should check for 0 value - It is assigned to an immutable variable and hence should be checked for zero value before assigning. - Frankencoin#L60

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L59-L62

6. CONSTANTS SHOULD BE DEFINED RATHER THAN USING MAGIC NUMBERS

  return minterReserveE6 / 1000000;
  

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L118

7. CORRECT THE TYPOS FOUND IN THE COMMENTS BELOW:

Here, we are also save, as 68 Bits would imply more than a trillion outstanding shares.

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

This limit could in theory be reached in times of hyper inflaction

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

case after their average holding duration is larger than or equal the required minimum

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

• helpes are necessary and to identify - Equity.sol#L207

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

• Design rule: Minters calling this method are only allowed to do so for tokens amounts they previously minted

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L188

• Under normal circumstances, this is just the reserver requirement multiplied by the amount.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L201

Following revert message in the MintingHub.openPosition() function can be more descriptive:

 require(_initialCollateral >= _minCollateral, "`must start with min col`");
 

Should be changed to:

 require(_initialCollateral >= _minCollateral, "`must start with atleast min col`");
 

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol#L109

8. Use Explicit Variable Declarations FOR DATA TYPES.

use `uint256` instead of `uint`

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

1. DO NOT PERFORM ARITHMETIC OPERATIONS IN CONSTANTS

Perform the arithmetic operations beforehand and assign the value to the constant to save gas.

uint256 public constant MIN_HOLDING_DURATION = 90*7200 << BLOCK_TIME_RESOLUTION_BITS; // Set to 5 for local testing

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

2. USE unchecked BLOCK TO SAVE GAS

In the Frankencoin.equity() function, the value (balance - minReserve) is returned. But prior to that a check is performed as follows:

  if (balance <= minReserve){
  

This check makes sure that (balance - minReserve) can never underflow and hence (balance - minReserve) can be unchecked to save gas.

 function equity() public view returns (uint256) {
    uint256 balance = balanceOf(address(reserve));
    uint256 minReserve = minterReserve();
    if (balance <= minReserve){
      return 0;
    } else {
      return balance - minReserve;
    }
  }

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L138-L146

3. Equity.canRedeem() external UTILITY FUNCTION CAN BE REMOVED SINCE canRedeem(address) IS A PUBLIC FUNCTION AND CAN BE DIRECTLY CALLED

function canRedeem() external view returns (bool){
    return canRedeem(msg.sender);
}

No use of the above function since the canRedeem(address) can be directly called since it is a public function.

function canRedeem(address owner) public view returns (bool) {
    return anchorTime() - voteAnchor[owner] >= MIN_HOLDING_DURATION;
}

This will help to save the deployment gas cost.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L127-L129 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L135-L137

4. IN Equity.votes(address, address[]) CONDUCT THE VARIABLE DECLARATIONS OUTSIDE THE for LOOPS TO SAVE GAS

    for (uint i=0; i<helpers.length; i++){
        address current = helpers[i];
		

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L192-L193 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Equity.sol#L312-L313

5. STATE VARIABLES USED FOR THE SECOND TIME INSIDE THE FUNCTION CAN BE CACHED TO SAVE GAS

function isMinter(address _minter) override public view returns (bool){ return minters[_minter] != 0 && block.timestamp >= minters[_minter]; }

In the Frankencoin.isMinter() function, the minters[_minter] state variable is accessed twice inside the function. Hence it can be cached into a memory variable. This memory variable can be used thereafter instead of using the minters[_minter] state variable.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L293-L295