Frankencoin - aria's results

QA report

Transfer and call function in ERC contract is not used but is vulnerable

location: In the Position contract, in the initializeClone function.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol - in line 162.

explanation:

The transfer and call function in the ERC-677 contract is not used for a reason, by calling to a function with the transferAndCall, the caller is vulnerable to reentrancy and DOS attacks since the receiver can control the process with his function and make the transaction as a whole fail on purpose or even to reenter the sender. So while this function is not used in the code a use of this function in contract to another is not suggested and dangerous.

In addition, this implementation may encourage security vulnerabilities in the contracts that interact with the coin.

// ERC-677 functionality, can be useful for swapping and wrapping tokens
function transferAndCall(address recipient, uint256 amount, bytes calldata data) external override returns (bool) {
	bool success = transfer(recipient, amount);
	if (success){
	    success = IERC677Receiver(recipient).onTokenTransfer(msg.sender, amount, data);
	}
	return success;
}

Shadowing of owner address in the initializeClone function

location: In the Position contract, in the initializeClone function.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol - in line 76.

explanation:

In the initializeClone function, in the position contract, the contract is using the name “owner” for an address. This is shadowing the original “owner” which was defined in the Ownable contract. Shadowing values can lead to unexpected results, in this part the wanted value is the one used and therefore it’s not a critical mistake.

function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
      if(_coll < minimumCollateral) revert InsufficientCollateral();
      setOwner(owner);
      
      price = _mint * ONE_DEC18 / _coll;
      if (price > _price) revert InsufficientCollateral();
      limit = _limit;
      mintInternal(owner, _mint, _coll);

      emit PositionOpened(owner, original, address(zchf), address(collateral), _price);
  }

contract Ownable {

    address public owner;
	//...
}

big logic error - creating a position with high challenge time:

location: In the MintingHub contract in the create position function.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/MintingHub.sol - in line 88.

explanation:

In the contract there is a weird exploit that will let us to create a position in such a way that challenging it will hurt the challenger. In this way we can put the contract in some sort of a hostage situation, when no one wants to challenge the position, yet if the position will pass, it could mint whatever it wants.

The way is by setting the challenging time to sometime far far away, if the challenging time is that big challenging the contract will result in the challenger’s funds being locked. This would also result in the owner’s funds being locked as well, but still creating a good position that no one will want to challenge is certainly a big threat to the contract and the community.

This should be fixed by bounding the value in the creation of the position.

No immediate incentive for use of the deny function:

location: In the deny functions in the Position contract and the denyMinter in the Frankencoin contract.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol - in line 152.

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol - in line 109.

explanation: Our deny functions are used by Equity holders to collapse positions and minters, the issue in the whole process is that the caller for the deny function might waste a lot of gas. Now the holders do have an incentive to deny bad positions and minter, but there is no incentive specifically to the sender. This means that while the community as a whole will want to deny, no one specifically will want to deny, since its better for them if others will send.

This might cause a weird game of chicken between equity holders when each one will want others to deny. The way to solve this problem will probably be by providing some incentive\ gas coverage to the caller to at least try and cover this.

We propose sending the gas cost of function in it end of it to the msg.sender / transaction.origin in the form of zchf tokens.

NOTE - calculate the delta of the gasleft in the beginning and the end of the funciton so the refund wont be exploitable.

[GAS-01] - Suggest the first Minter of Frankencoin in the constructor and save some gas in the suggestMinter() Function

before:

function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
   if (_applicationPeriod < MIN_APPLICATION_PERIOD && totalSupply() > 0) revert PeriodTooShort();
   if (_applicationFee < MIN_FEE  && totalSupply() > 0) revert FeeTooLow();
   if (minters[_minter] != 0 && minters[_minter] <= block.timestamp + _applicationPeriod) revert AlreadyRegistered();
   _transfer(msg.sender, address(reserve), _applicationFee);
   minters[_minter] = block.timestamp + _applicationPeriod;
   emit MinterApplied(_minter, _applicationPeriod, _applicationFee, _message);
}

after:

function suggestMinter(address _minter, uint256 _applicationPeriod, uint256 _applicationFee, string calldata _message) override external {
   if (_applicationPeriod < MIN_APPLICATION_PERIOD) revert PeriodTooShort();
   if (_applicationFee < MIN_FEE) revert FeeTooLow();
   if (minters[_minter] != 0) revert AlreadyRegistered();
   _transfer(msg.sender, address(reserve), _applicationFee);
   minters[_minter] = block.timestamp + _applicationPeriod;
   emit MinterApplied(_minter, _applicationPeriod, _applicationFee, _message);
}

in constructor -

constructor(uint256 _minApplicationPeriod, address _initialMinter) ERC20(18){
   MIN_APPLICATION_PERIOD = _minApplicationPeriod;
   reserve = new Equity(this);

   minters[_initialMinter] = block.timestamp;
   emit MinterApplied(_initialMinter, 0, 0, "Initial minter"); 
}

scope - https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Frankencoin.sol#L83-L90

[GAS - 02] - function isClosed is wasting gas on deployment - both fields (collateralBalance which is a wrapper of IERC20.balanceof(address(this))) and minimumCollateral are public.

If this function is only used on front end there is no reason to waste gas on it

before:

function  isClosed() public  view  returns (bool) {
	return  collateralBalance() < minimumCollateral;
}

after:

//nothing///

scope: https://github.com/code-423n4/2023-04-frankencoin/blob/1022cb106919fba963a89205d3b90bf62543f68f/contracts/Position.sol#L360-L362

[GAS - 03] - For medium to large amount of helpers, it is better to test if someone already voted by using a storage map. reducing the time complexity to O(n) from O(n^2)

before:

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));
            for (uint j=i+1; j<helpers.length; j++){
                require(current != helpers[j]); // ensure helper unique
            }
            _votes += votes(current);
        }
        return _votes;
}

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

		for (uint i = 0; i<helpers.length; i++){
			counted[helpers[i] = false;
		}
		counted[sender] = false;
        return _votes;
}

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