code-423n4 / 2022-02-foundation-findings

0 stars 0 forks source link

QA report #11

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/FETH.sol#L232-L242

Vulnerability details

Impact

The function depositFor(address account) allows account to be zero. If a user / program accidentally sets this value to be zero then the msg.value will be added to accountToInfo[address(0)] and the funds will not be recoverable.

Proof of Concept

  function depositFor(address account) public payable {
    if (msg.value == 0) {
      revert FETH_Must_Deposit_Non_Zero_Amount();
    }
    AccountInfo storage accountInfo = accountToInfo[account];
    // ETH value cannot realistically overflow 96 bits.
    unchecked {
      accountInfo.freedBalance += uint96(msg.value);
    }
    emit Transfer(address(0), account, msg.value);
  }

Recommended Mitigation Steps

Consider adding a check to ensure account is non zero to prevent accidental loss of funds.

For example

  function depositFor(address account) public payable {
    if (account == address(0)) {
      revert FETH_Must_Deposit_Non_Zero_Account();
    }
    ...
HardlyDifficult commented 2 years ago

Duplicate of #2

This is a great suggestion to help avoid user error, and we have PR'd the recommended change.

We believe the original reported severity of 1 (Low Risk) is a better fit for this issue. There is no risk to the protocol or other users here. The concern is about users making a mistake that prevents them from accessing their own funds.

alcueca commented 2 years ago

Agree with the downgraded severity.

alcueca commented 2 years ago

QA Report score: 10

alcueca commented 2 years ago

Removing the duplicate because it doesn't apply to QA Reports

CloudEllie commented 2 years ago

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "depositFor() Does Not Verify account is Non Zero Potentially Losing Funds."