Frankencoin - santipu_'s results

Lines of code

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Equity.sol#L262-L264

Vulnerability details

Impact

In the Equity contract, the calculateShares function returns a wrong value when equity is less than investment.

calculateShares is a view function that returns the number of shares that will be minted corresponding an investment of ZCHF tokens. This function calls directly calculateSharesInternal that calculates the shares to be minted.

The function that actually makes the mint of shares happen is onTokenTransfer. In this function there is a check to ensure that if equity <= amount the shares minted will always be 1000e18. This check is to assign 1000e18 shares minted to the first depositor.

The problem is that the calculateShares function doesn't account for this so will return a different amount of shares when the equity is less than investment. This can happen before the first deposit or in a situation of low liquidity when the equity is low.

Proof of Concept

• calculateShares function: https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Equity.sol#L262-L264
• Check in onTokenTransfer: https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Equity.sol#L247

Tools Used

Manual review

Recommended Mitigation Steps

Check in calculateShares the same condition as in onTokenTransfer. Example:

function calculateShares(uint256 investment) public view returns (uint256) {
    uint256 equity = zchf.equity();
    return equity <= investment ? 1000 * ONE_DEC18 : calculateSharesInternal(equity, investment);
}

Lines of code

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Frankencoin.sol#L146-L151

Vulnerability details

Impact

In the Frankencoin system a minter that hasn't been denied during the application period can't be revoked in case of a private keys leak or a malicious minter.

Currently, when a minter is suggested and not denied during the application period, it will be able to mint tokens and transfer tokens of users as it please so it has to be a trusted party. The problem is that in case the private keys of a minter are leaked or this minter turns out to be malicious, it has the power to mint mint infinite tokens or burn them. The FPS holders should have a mechanism to revoke the privileges of a minter if any of these happens.

Proof of Concept

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Frankencoin.sol#L146-L151

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a mechanism to revoke the privileges of a minter in case something unexpected happens in order to prevent the destruction of the Frankencoin ecosystem.

Wrong comments at Ownable contract

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Ownable.sol#L16-L17

In the Ownable contract, that is a modified copy from the Openzeppelin Ownable contract, the comments state that the owner will be by default the contract deployer. This statement is false because in the contract there's no constructor so the contract that inherit form this one must set the owner manually.

This issue has no real impact because the Position contract sets the owner in the constructor but this comment can lead to misunderstanding with other devs or with future versions of Frankencoin leading to a contract without owner.

checkQualified won't revert when there are no shares minted or if it's called in the same block as the first shares are minted

https://github.com/Frankencoin-ZCHF/FrankenCoin/blob/main/contracts/Equity.sol#L209-L212

In the Equity contract, the checkQualified function should revert when the sender doesn't hold at least the 3% of the votes. This doesn't happen when there're not shares minted of when the function is called in the same block as the transaction that minted the first shares.

This is because the function calls totalVotes, and this function makes the following calculation: totalVotesAtAnchor + totalSupply() * (anchorTime() - totalVotesAnchorTime) So, when there are no shares minted, totalVotesAtAnchor and totalSupply will be zero making the function to not revert as it should. Also, in the same block as the first deposit, totalVotesAtAnchor will still be zero and anchorTime() - totalVotesAnchorTime will also result in zero making the function to not revert as it should.