search

code-423n4 / 2022-12-prepo-findings

0 stars 1 forks source link

User may burn Short or Long tokens, losing any claim of collateral tokens and causing leak of value. #304

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/LongShortToken.sol#L8

Vulnerability details

Description

LongShortToken is the contract used for Long and Short tokens created in PrePOMarket. Unfortunately, they inherit from ERC20Burnable:

contract LongShortToken is ERC20Burnable, Ownable {
  constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}
  function mint(address _recipient, uint256 _amount) external onlyOwner { _mint(_recipient, _amount); }
}

They inherit the public burn function:

function burn(uint256 amount) public virtual {
    _burn(_msgSender(), amount);
}

Long and Short tokens are never meant to be burnt directly! The only intended burn flow is when redeeming collateral tokens in the PrePOMarket:

longToken.burnFrom(msg.sender, _longAmount);
shortToken.burnFrom(msg.sender, _shortAmount);
uint256 _collateralAfterFee = _collateralAmount - _actualFee;
collateral.transfer(msg.sender, _collateralAfterFee);

User approves the burn to PrePOMarket, it burns the tokens on their behalf and transfers the corresponding amount of collateral tokens. If user burns their tokens directly, they instantly lose their claim in collateral. Users might not be aware of this destructive behavior and expect burn() to compensate them in collateral tokens.

Impact

User may burn Short or Long tokens, losing any claim of collateral tokens and causing leak of value.

Tools Used

Manual audit

Recommended Mitigation Steps

Most safe behavior would be to require that the message sender in burn / burnFrom is owner, which is already defined to be market:

// Already happens in createMarket
_longToken.transferOwnership(address(_newMarket));
_shortToken.transferOwnership(address(_newMarket));

Judge note

This is a personal preference sort of issue. I know some judges treat this sort of mistake as overrecklessness of user. Others treat it as a legitimate leak of value. I'm on the fence on this one which is why I'm reporting it as M.

Picodes commented 1 year ago

Duplicate of #309. To me this falls within QA, as unfortunately I am among the ones that consider this "a sort of mistake as overrecklessness of user" :)

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-prepo-findings/issues/315