Joyn contest - pedroais's results

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/ERC721Payable.sol#L54

Vulnerability details

Impact

User can mint and if he doesn't have enough tokens the transfer will fail and if the token doesn't revert it will return false and the minting will continue. The user will mint everything for free.

Proof of Concept

Some ERC20 tokens like USDT don't revert on failure and instead return false. This is why projects that use arbitrary tokens should use the SafeTransfer library to ensure compliance with the ERC20 interface. https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/ERC721Payable.sol#L54

Recommended Mitigation Steps

Require the transfer to succeed or use safeMath

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/Splitter.sol#L149

Vulnerability details

Impact

One co-creator with a small share can get 100% of the funds by calling the incrementWindow() function from an attacker contract that mimics RoyaltyVault. He can then create one or multiple fake windows and claim them to get the full balance of the splitter.

Proof of Concept

Proof of concept :

Bob and Attacker are co-creators and they have a 50/50 split of the revenue. The splitter has a balance of 10 weth so each should get 5. The 10 weth was deposited in one window.

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/Splitter.sol#L149

Since incrementWindow() is a public function attacker can call it with a contract that implements some functions from the royaltyVault interface. Attack : -The attacker contract calls incrementWindow with a royaltyAmount equal to the token balance of the splitter (10 weth) -The attacker contract is crafted to mimic a RoyaltyVault to pass the requires made to msg.sender -royaltyAmount is equal to wethBalance of the splitter (10) -A new window is created and royaltyAmount is pushed into the array -Since this was done from the attacker contract those 10 weth were never sent but the window was still created

An attacker can then claim 50% of every window using his merkle proof. He can claim all the different windows but can't claim a window twice. He created a fake window with 10 eth so he can claim the real window and get his 50% (5 eth) and then claim the fake window to get the other 5 eth. Now the contract has no funds and Bob won't be able to claim his share. https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/splits/contracts/Splitter.sol#L112 Attacker got 100% of the funds instead of his 50%

Recommended Mitigation Steps

Always check that the sum of all windows is equal or less than balance.

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L78

Vulnerability details

Impact

Anyone can create a collection so the owner is not a trusted actor. Someone can create a collection with a maxSupply and then call initialize again to increase it. Also, the creator can use it to change price and front-run a mint with an arbitrary bigger price. Also, the splitter contract address can be changed to anything which allows the owner to get all funds.

Proof of Concept

The initializer function in CoreCollection.sol doesn't require initialized == false

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreCollection.sol#L78

Recommended Mitigation Steps

require(initialized == false)