Foundation contest - 0x1f8b's results

Yes, this is a good best practice to be following. We will add these functions in the near future. In the meantime we've added an inline comment about the potential risk.

At first we don't expect there will be much interest in using FETH outside of Foundation itself. Thanks to the trusted relationship with the market contract, approvals are not necessary. So this is not an issue that should come up in the near term.

We are not fixing this immediately because it introduces a risk associated with another contract our users have deployed, that contract was not part of the contest repo. We are looking to patch that vulnerability and then will add increaseAllowance to FETH.

In a way, this is a feature not a bug. The use case for this signature is meant to be used only when the private key for their original account has been compromised. When that occurs, that account should never be used again regardless of the network. So when Foundation supports multiple chains, we would want the ability to migrate their account on all networks and it would be most convenient to do that with only 1 signature required from the user.

We acknowledge that including the chainId with signatures is generally a best practice, and may consider changing that in the future.

We believe this is a 1 (Low Risk) issue because of the point above, but also the signature is only usable when broadcasted by a Foundation admin which should not be abusing the capability.

Including the chainId with signatures is a best practice, but only when the signature is supposed to be valid only on one network. In this case if the signature should be used across all networks, there is no need to restrict it.

However, the lack of natspec of other documentation describing this makes this a valid issue.

The fact that the feature can only be abused by a privileged account reduces its severity by one, to Sev 1.

QA Report score: 10

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 "Signature reusable over different networks."

Thanks for the feedback.

1. We decided to keep returning the unpacked version because this approach shields consumers from the slot packing strategy we use, and allows us to potentially make other additions or changes to the struct without impacting the external ABI. Ideally we would be more consistent here and do the same when returning the ReserveAuction struct - however there is a 3rd party contract which has already created a dependency on the current ABI.

2. We agree with this change as it improves readability and will switch to delete pendingWithdrawals[user] here.

Wouldn't it be cheaper to do Offer memory offer = and then return (offer.buyer, offer.expiration, offer.amount)? The slot is warm, but still you should save about 300 gas.

Score: 300