code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L194-L197 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Frankencoin.sol#L223-L229

Vulnerability details

Impact

In the Frankencoin.burn() function, the minter is allowed to burn the debt by burning ZCHF tokens and freeing and donating the respective minter reserve amount to the pool share holders.

In the comment for the function implementation it says the following: Minters calling this method are only allowed to so for tokens amounts they previously minted with the same _reservePPM amount.

But the above check for the same _reservePPM amount is never implemented in the Frankencoin.burn() function. Hence the minter can input any reservePPM as the input parameter and increase the equity portion of the reserve thus increasing the profit attributable to the pool share holders.

If the minter himself is a pool share holder, he can use this to increase his portion of the profit.

Even if the minter is not malicious, there is a possiblity he could input higher _reservePPM by mistake, which could lead to increased profit attributable to the pool share holders. This is not the intended behaviour of the protocol.

Similar issue is found in the Frankencoin.burnFrom() function as well. Here in the comment it is mentioned that same _reservePPM used for the mint by the caller, should be used as _reservePPM for this function as well.

But it is not implemented within the function logic. Hence if by mistake the minter uses a higher _reservePPM as the input parameter, the assigned value of the function will be higher than expected value. Hence more ZCHF will be transfered to the payer from the minter reserve.

Hence the payer will get unfair advantage since he will repay less amount of ZCHF tokens, more tokens will be paid from the minter reserve at the expense of other payers.

Proof of Concept

   function burn(uint256 amount, uint32 reservePPM) external override minterOnly {
      _burn(msg.sender, amount);
      minterReserveE6 -= amount * reservePPM;
   }

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

   function burnFrom(address payer, uint256 targetTotalBurnAmount, uint32 _reservePPM) external override minterOnly returns (uint256) {
      uint256 assigned = calculateAssignedReserve(targetTotalBurnAmount, _reservePPM);
      _transfer(address(reserve), payer, assigned); // send reserve to owner
      _burn(payer, targetTotalBurnAmount); // and burn the full amount from the owner's address
      minterReserveE6 -= targetTotalBurnAmount * _reservePPM; // reduce reserve requirements by original ratio
      return assigned;
   }

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

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

It is recommended to store the previous _reservePPM for each debt position minted in a mapping and check that value against the input parameter _reservePPM to the Frankencoin.burn() function, when trying to repay the debt by burning ZCHF amount.

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as primary issue

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as low quality report

0xA5DF commented 1 year ago

It's the minter's responsibility to burn the tokens with the same reserve PPM as when it was created

c4-judge commented 1 year ago

hansfriese changed the severity to QA (Quality Assurance)