First Flight #15: Mondrian Wallet
First Flight #15
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Wrong Implementation of ERC721

4rdiii

[H-2] Wrong Implementation of ERC721

Description: The contract inherits ERC721 implmentation, but these account contracts are meant to be deployed once for every user cause the user should be owner of the contract. as a result of this every contract would be a ERC721 token with zero tokens in it. Also _mint function is never called so there will be no NFTs generated in the first place so if we add the _mint to constructor we also will have to somehow fix the many implementations problem.

@> contract MondrianWallet is Ownable, ERC721, IAccount {
.
.
.
constructor(
address entryPoint
) Ownable(msg.sender) ERC721("MondrianWallet", "MW") {
i_entryPoint = IEntryPoint(entryPoint);
@> _mint(owner(), 1); // this was added to fix zero nfts minted problem
}
​

Impact: This will cause all Nfts having same URIs and also the nft collection will be seperated and meaningless.

Proof of Concept: if we add the _mint to constructor we can then use this test suit written using hardhat-foundry library. To prove the problem:

contract MWTest is Test {
MondrianWallet mw;
MockERC20 erc20;
MockEntryPoint ep;
address user = makeAddr("user");
address secondUser = makeAddr("user2");
uint256 constant MINT_AMOUNT = 10 ether;
function setUp() public {
if (block.chainid == 1) {
//eth mainnet
ep = MockEntryPoint(
payable(address(0x0000000071727De22E5E9d8BAf0edAc6f37da032)) // entrypoint on mainnet
);
} else {
ep = new MockEntryPoint();
}
vm.prank(user);
mw = new MondrianWallet(address(ep));
erc20 = new MockERC20();
vm.deal(user, MINT_AMOUNT);
}
function testNFTImplementation() public {
vm.prank(secondUser);
MondrianWallet mw2 = new MondrianWallet(address(ep));
address ownerOf2 = mw2.ownerOf(1);
address ownerOf1 = mw.ownerOf(1);
assertEq(ownerOf2, secondUser);
assertEq(ownerOf1, user);
assert(ownerOf2 != ownerOf1);
}
}

Recommended Mitigation: To fix this we can use a factory contract to deploy the Wallet instances and have the factory be also the ERC721 implmentation, this way also there would not be an issue with minting cause we can mint the nft when the createAccount function is called.
here is a simple factory from eth-infinitism github altered to have ERC721 in it.

// SPDX-License-Identifier: GPL-3.0
pragma solidity ^0.8.23;
​
import "@openzeppelin/contracts/utils/Create2.sol";
import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "./SimpleAccount.sol"; // this would be the MondrianWallet.sol
​
/**
* A sample factory contract for SimpleAccount
* A UserOperations "initCode" holds the address of the factory, and a method call (to createAccount, in this sample factory).
* The factory's createAccount returns the target account address even if it is already installed.
* This way, the entryPoint.getSenderAddress() can be called either before or after the account is created.
*/
contract SimpleAccountFactory is ERC721 {
SimpleAccount public immutable accountImplementation;
​
constructor(IEntryPoint _entryPoint) {
accountImplementation = new SimpleAccount(_entryPoint);
}
​
/**
* create an account, and return its address.
* returns the address even if the account is already deployed.
* Note that during UserOperation execution, this method is called only if the account is not deployed.
* This method returns an existing account address so that entryPoint.getSenderAddress() would work even after account creation
*/
function createAccount(address owner,uint256 salt) public returns (SimpleAccount ret) {
address addr = getAddress(owner, salt);
uint256 codeSize = addr.code.length;
if (codeSize > 0) {
return SimpleAccount(payable(addr));
}
ret = SimpleAccount(payable(new ERC1967Proxy{salt : bytes32(salt)}(
address(accountImplementation),
abi.encodeCall(SimpleAccount.initialize, (owner))
)));
_mint(address(ret),1);
}
​
/**
* calculate the counterfactual address of this account as it would be returned by createAccount()
*/
function getAddress(address owner,uint256 salt) public view returns (address) {
return Create2.computeAddress(bytes32(salt), keccak256(abi.encodePacked(
type(ERC1967Proxy).creationCode,
abi.encode(
address(accountImplementation),
abi.encodeCall(SimpleAccount.initialize, (owner))
)
)));
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

Extremely Wrong Implementation of ERC721

The Wallet doesn't end up owning any nft

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.