code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

withdrawCollateral allows position owners to withdraw collateral from the position - as long as it is still collateralized afterwards.

File: Position.sol
263:     function withdrawCollateral(address target, uint256 amount) public onlyOwner noChallenge noCooldown {
264:         uint256 balance = internalWithdrawCollateral(target, amount);
265:         checkCollateral(balance, price);
266:     }
267: 
268:     function internalWithdrawCollateral(address target, uint256 amount) internal returns (uint256) {
269:         IERC20(collateral).transfer(target, amount);
270:         uint256 balance = collateralBalance();
271:         if (balance < minimumCollateral){
272:             cooldown = expiration;
273:         }
274:         emitUpdate();
275:         return balance;
276:     }

The issue is the use of IERC20.transfer(): tokens that do not fully comply will result in the call reverting.

Impact

Tokens that return a boolean for transferFrom(), but not transfer(), will result in the impossibility to withdraw collateral from a position, on top of reverting all calls to end() for successful challenges. BNB is a token with this configuration (4th biggest marketcap).

Note: the documentation mentions collateral tokens must adhere to the ERC-20 standard, which means the token aforementioned is technically not meant to be supported. It however represents a large portion of the overall tokens marketcap, and not supporting it would affect the availability of Frankencoin.

Note 2: the known issues report mentions the calls to transfer() reverting (ie no direct loss of funds), while this reports highlights the fact that using BNB in a Position will result in locked collateral.

Proof Of Concept

Run the test test_Audit_Pos_Fail_BNB in the following gist: https://gist.github.com/joestakey/fbc954de53b7e018f5121212488e847c You will need to add an Ethereum key in foundry.toml to run the test using a fork of mainnet.

Tools Used

Manual Analysis, Foundry

Mitigation

Use a safeERC20 library - such as OpenZeppelin's one to perform collateral transfers.

c4-pre-sort commented 1 year ago

0xA5DF marked the issue as duplicate of #777

c4-judge commented 1 year ago

hansfriese marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-04-frankencoin-findings