Loss of user funds if user approves malicious transaction
Description
Trace: createContract() of Escher721.Factory.sol calls initialize() of Escher721.sol
The initialize() function sets tokenUriDelegate to the passed in _uri address. There's no input validation in Escher721Factory.sol or Escher721.sol for _uri in either contracts.
A malicious creator or a creator victim of a dusting attack (real _uri looks similar to fake _uri) can pass in a _uri that's an exploitative contract. When the public tokenURI() function is called, it calls tokenURI() on the malicious contract.
Proof of Concept
Eve the creator of an Escher721 collection falls victim to a dusting attack. A similar looking address to her _uri sent her some tokens
Eve mistakenly copies the wrong address (the malicious dusting contract) to the createContract() _uri parameter.
When Bob an unsuspecting user calls tokenURI() it calls tokenURI() on the malicious contract
The malicious contract asks to sign a transaction that approves all of Bob's WETH to spend
Bob doesn't know what he signs
Malicious contract steals all of Bob's WETH
Tools Used
Manual review
Recommended Mitigation Steps
Use stricter validation for _uri. Consider other design choices that doesn't allow calling an external contract set by the creator.
Note
I'm sorry, didn't had time to investigate this more, but wanted to include it anyway.
Lines of code
https://github.com/code-423n4/2022-12-escher/blob/main/src/Escher721.sol#L32-L46
Vulnerability details
Impact
Loss of user funds if user approves malicious transaction
Description
Trace: createContract() of Escher721.Factory.sol calls initialize() of Escher721.sol
The initialize() function sets tokenUriDelegate to the passed in _uri address. There's no input validation in Escher721Factory.sol or Escher721.sol for _uri in either contracts. A malicious creator or a creator victim of a dusting attack (real _uri looks similar to fake _uri) can pass in a _uri that's an exploitative contract. When the public tokenURI() function is called, it calls tokenURI() on the malicious contract.
Proof of Concept
Tools Used
Manual review
Recommended Mitigation Steps
Use stricter validation for _uri. Consider other design choices that doesn't allow calling an external contract set by the creator.
Note
I'm sorry, didn't had time to investigate this more, but wanted to include it anyway.