Seaport orders will not work with USDT

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L121-L123

Vulnerability details

Impact

It will be impossible to execute ERC20 orders through Seaport that use USDT, as the transaction will fail every time. CreateOfferer will be unusable for any transaction that uses USDT.

Proof of Concept

As per ERC20 specification, approve normally returns a boolean:

function approve(address spender, uint256 value) external returns (bool);

However, USDT deviates from this standard and its approve method doesn't have any return value.

If USDT is used when creating a delegate token through Seaport, the following line will revert, as it expects return data, but it doesn't receive any:

if (!IERC20(erc20Order.info.tokenContract).approve(address(delegateToken), erc20Order.amount)) {
    revert Errors.ERC20ApproveFailed(erc20Order.info.tokenContract);
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/CreateOfferer.sol#L121-L123

Tools Used

Manual review

Recommended Mitigation Steps

Consider using Solmate's safeApprove to avoid this issue, as it's compatible with USDT's approve.

Assessed type

ERC20

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

0xfoobar (sponsor) acknowledged

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Have gone and checked Seaport and USDT was never used

I am extremely disappointed that the Bot chosen for the Report didn't find this

4nalyzer has been reporting this issue for over a year: https://github.com/Picodes/4naly3er

[DadeKuma]

I am extremely disappointed that the Bot chosen for the Report didn't find this

Eh, I've removed this finding manually as it thought it was a false positive but then double-checked again when the contest started and it was valid.

Have gone and checked Seaport and USDT was never used

I'm not sure what this means, it seems like any token can be used as a payment, including USDT?

[GalloDaSballo]

To me the above is an admission of cheating

[DadeKuma]

I'm honestly confused how would this be considered cheating? Note that the Bot chosen for the Report was mine and it didn't submit this issue

[GalloDaSballo]

• Your bot missed a common issue that has been OOS since 4nalyz3r was used
• You sent the finding as Med Severity
• You had access to other Bot reports
• You said above that your Bot would have produced the output and you manually removed it

[DadeKuma]

I'm sorry, but I still don't see how this is cheating, as I don't think I violated any rules?

1. My bot submitted this as a Med severity
2. I removed it because I thought it could have been a false positive. The reason is that I would get harshly penalized during the bot race if it wasn't valid Med. Please note that during the race, we have very limited time, and it's possible to mistakenly remove true positives
3. I participated in this contest and checked out this part of the code, realizing that it was an issue. You can be sure that I've really participated, as I've submitted a lot of other issues other than this one
4. I've re-checked the bot report, and this specific issue was not out of scope. Please note that any other warden could have submitted this issue (and they did)

I had no ill intentions, so it's flabbergasting to have been accused of cheating. Could you please send any links with the rules you are implying that I've violated?

[GalloDaSballo]

As we discussed privately these are the rules that I believe you violated: https://www.notion.so/Code-of-Professional-Conduct-657c7d80d34045f19eee510ae06fef55

Highlighting

Respect your privileged access and position within C4 and avoid using this position against the competitive interests of C4.

Finding such as these are used to mock C4 as a respectable platform, bot races were added to prevent that, and in this instance have failed.

Staff has all the information necessary to make their determination and at this time I'm maintaining a QA Severity given the fact that USDT is not a token commonly used for NFT transactions

[GalloDaSballo]

After asking 2 additional judges, they agree with my decision on severity citing that the token is not explicitly meant to be used and that funds are not lost

[sockdrawermoney]

Just a personal note here—not weighing in on the specific issue.

There’s a lot of intense feelings in this thread and I understand why.

I fully accept @GalloDaSballo’s argument that this is an undesired circumstance and I’m with Alex in feeling unsettled by the possibility of awarding findings from circumstantial or logical inconsistencies in rules and that result in loopholes.

At the same time, I do not believe that the combination of available evidence and @DadeKuma’s past track record as a great community contributor point to a corrupt motive here.

This issue has clearly surfaced some assumptions and open loops in our rules. I know @CloudEllie has had her attention on this and I appreciate it and trust she’ll help us get what we need to figure it out. Let’s work together to make those things clear going forward so the ambiguities of this circumstance aren’t possible in the future.

Thanks, all.

[DadeKuma]

I can understand Alex's frustration because this is a very common issue that should have been included in the bot report.

The main reason why 4nalyzer never missed this is that there weren't any penalties for invalid findings at that time. There was no reason to not include it in the report, no matter if it was valid or invalid.

The current meta in bot races is to generate a report, manually filter it to remove potential invalid findings, and submit the report within one hour. Without manually filtering, a bot crew will definitely lose the race; we are incentivized to do this manual filtering.

A solution to this problem could be to automate submissions (so that no one can manually filter) and make all bot reports public and OOS. I know you are already working on the latter, but the former will prevent this problem from arising in the future.