Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solmate's ERC20 does not check for token contract's existence, which opens up possibility for a honeypot attack #48

Open
code423n4 opened this issue Nov 6, 2022 · 5 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden M-02 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report

Comments

@code423n4
Copy link
Contributor

code423n4 commented Nov 6, 2022

Lines of code

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L163

Vulnerability details

Description

When bidding, the contract pulls the quote token from the bidder to itself.

https://github.com/code-423n4/2022-11-size/blob/main/src/SizeSealed.sol#L163

SafeTransferLib.safeTransferFrom(ERC20(a.params.quoteToken), msg.sender, address(this), quoteAmount);

However, since the contract uses Solmate's SafeTransferLib

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

Therefore if the token address is empty, the transfer will succeed silently, but not crediting the contract with any tokens.

This error opens up room for a honeypot attack similar to the Qubit Finance hack in January 2022.

In particular, it has became popular for protocols to deploy their token across multiple networks using the same deploying address, so that they can control the address nonce and ensuring a consistent token address across different networks.

E.g. 1INCH has the same token address on Ethereum and BSC, and GEL token has the same address on Ethereum, Fantom and Polygon. There are other protocols have same contract addresses across different chains, and it's not hard to imagine such thing for their protocol token too, if any.

Proof of Concept

Assuming that Alice is the attacker, Bob is the victim. Alice has two accounts, namely Alice1 and Alice2. Denote token Q as the quote token.

  1. Token Q has been launched on other chains, but not on the local chain yet. It is expected that token Q will be launched on the local chain with a consistent address as other chains.
  2. Alice1 places an auction with some baseToken, and a token Q as the quoteToken.
  3. Alice2 places a bid with 1000e18 quote tokens.
    • The transfer succeeds, but the contract is not credited any tokens.
  4. Token Q is launched on the local chain.
  5. Bob places a bid with 1001e18 quote tokens.
  6. Alice2 cancels her own bid, recovering her (supposedly) 1000e18 refund bid back.
  7. Alice1 cancels her auction, recovering her base tokens back.

As a result, the contract's Q token balance is 1e18, Alice gets away with all her base tokens and 1000e18 Q tokens that are Bob's. Alice has stolen Bob's funds.

Tools Used

Manual review

Recommended Mitigation Steps

Consider using OpenZeppelin's SafeERC20 instead, which has checks that an address does indeed have a contract.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 6, 2022
code423n4 added a commit that referenced this issue Nov 6, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Nov 9, 2022

0xean marked the issue as duplicate

@c4-judge
Copy link
Contributor

0xean marked the issue as selected for report

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Nov 24, 2022
@c4-judge
Copy link
Contributor

0xean changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Nov 28, 2022
@c4-judge
Copy link
Contributor

0xean marked the issue as satisfactory

@C4-Staff C4-Staff added the M-02 label Dec 6, 2022
@C4-Staff C4-Staff removed selected for report This submission will be included/highlighted in the audit report duplicate-318 labels Jun 26, 2023
@C4-Staff
Copy link
Contributor

captainmangoC4 marked the issue as selected for report

@C4-Staff C4-Staff added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue edited-by-warden M-02 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

3 participants