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

Anyone can get stuck ETH and ERC20 #109

Closed
code423n4 opened this issue Nov 12, 2022 · 3 comments
Closed

Anyone can get stuck ETH and ERC20 #109

code423n4 opened this issue Nov 12, 2022 · 3 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 duplicate-277 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-11-looksrare/blob/e3b2c053f722b0ca2dce3a3eb06f64859b8b7a6f/contracts/LooksRareAggregator.sol#L108-L109

Vulnerability details

Description

LooksRareAggregator.execute() at the end of execution send all unspent ETH and ERC20 back to actor. But it send all amount lying on the contract which may be bigger than initially sent by actor. So anyone seeing that there are some ETH or ERC20 tokens stuck on contract may just send with a call of ERC20EnabledLooksRareAggregator.execute() 1 token of each ERC20 with positive contract balance, some dummy trade order (which may fail) and isAtomic=false flag and get back all funds on contract in ETH and ERC20.
There are special functions to rescue stuck ETH and ERC20 (TokenRescuer.rescueERC20() and TokenRescuer.rescueETH()) which can be called only by owner. So obviously by business-logic anyone shouldn't be able to get stuck ETH and ERC20.

Recommendation

Save initial amount and spent amount of ETH and ERC20 during fulfillment of all orders. Return only unused part of initially sent funds.

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

Picodes marked the issue as duplicate of #277

@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 Dec 16, 2022
@c4-judge
Copy link
Contributor

Picodes changed the severity to 2 (Med Risk)

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

Picodes marked the issue as satisfactory

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 duplicate-277 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants