Rigor Protocol contest - byndooa's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L508-L552

Vulnerability details

Impact

Data+signature for previous calls of Community.escrow() can be called one more time by anyone, to reduce project's debt. It breaks all balances significantly between parties

Proof of Concept

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Community.sol#L508-L552 Community.escrow() reduces debt for a project. To call the function data+signature is required. If this transaction happened once, right to reduce debt are in onchain history. Anyone can call Community.escrow() now with these disclosed data+signature to reduce debt more and more.

Tools Used

Hardhat

Recommended Mitigation Steps

Add nonce in data or store used data+signature

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L386-L490 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L359

Vulnerability details

Impact

I have already commited the issue with the repeated calls of Project.changeOrder() by anyone. Here is a worse usecase - past subcontractor can steal funds from new subcontractor for task completed.

Subcontractor's fund for completion can be stolen by a previous subcontractor (if any). Bad subcontractor can frontrun setComplete() transaction, if he calls Project.changeOrder() with already disclosed data+signature, set himself an active subcontractor and receives funds from task completed.

Proof of Concept

Project.changeOrder() is used to change subcontractors for tasks and costs for tasks. Once this transaction is in history - it can be repeated by anyone using the same data+signature. And here is setComplete() function. https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L330-L359 It makes a task completed and transfers money to a subcontractor. The attack is:

1. setComplete() is called to pay to new subcontractor for his completed task, transaction is in mempool
2. past subcontractor (previously kicked) picks some changeOrder() in history, where he was a subcontractor
3. past subcontractor frontruns #1, he transact with changeOrder()
4. past subcontractor is now an active subcontractor
5. setComplete() happens, bad-subcontractor-fronrunner receives funds

Tools Used

Hardhat

Recommended Mitigation Steps

Add nonce in data or store used data+signature

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L232

Vulnerability details

Impact

Anyone can create project. Creating project = mining NFT. At the beginning their owners are the same. But NFT means nothing, as with transferring NFT the ownership of the Project is not transferred (there is no way to change the builder for a project). It mean total dissinchronization between NFTs and Projects.

Proof of Concept

HomeFi.CreateProject() NFT allows transferring as it imports ERC721Upgradeable.sol Like in a deployed contract: https://rinkeby.etherscan.io/address/0x9361613391Ae3de8112F9571193268363B8704C5#writeProxyContract But Project.builder cannot be changed

Tools Used

Hardhat

Recommended Mitigation Steps

Dissable transfership of change builders NFT is transferred.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L162-L182 https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L266-L293

Vulnerability details

Impact

It is expected that the data+signature for updateProjectHash() will be triggered once and for an exact project. But anyone can change a project hash, if he will find some easy matching parameters for another project.

Proof of Concept

Requirements to execute function for different projects with same inputs:

• nonce is the same (they are likely to low or zero for every project)
• both projects don't have contractors but the same builder ( to pass checkSignature () )
• OR both projects have the same contractor delegated ( to pass checkSignature () )
• OR both projects have the same pair of builder+contractor ( to pass checkSignature () )

The same thing is for Project.updateTaskHash()

Tools Used

Hardhat

Recommended Mitigation Steps

Add project_address to data like in Project.changeOrder()

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFiProxy.sol#L58-L90

Vulnerability details

Impact

Anyone can frontrun initialization and feed any inputs. The project will not work. Some of inputs may allow project working, but can steal funds later (like HomeFi.treasury, HomeFi address in all contracts. Attacker can deploy his own contract with the hidden behavior, feed them to init() (as if it s a hobeFi contract) and steal funds when they appear on Community or Project contracts.

Proof of Concept

Below - Etherscan addresses with the transactions of contracts. Everywhere we see initialize() as a method for the first transaction - it means init() is a separate transaction after contract deploy.

[HomeFiProxy.sol] - deploy by EOA + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0x5d00B82009E20E7fa4C1506d3D785702094C3828

[HomeFi.sol] - deployed by HomeFiProxy + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0x9361613391Ae3de8112F9571193268363B8704C5

[Community.sol] - deployed by HomeFiProxy + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0xDd725766c86B0BEe05A89b5587aAd4da3ACE37Ee

[Disputes.sol] - deployed by HomeFiProxy + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0x448d745DeA689388CbAd5b1Bea8c204E9464C8C1

[ProjectFactory.sol] - deployed by HomeFiProxy + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0xD2DE87906f78002058587123c31EDC43f94a6Bba

[DebtToken.sol] - deployed by HomeFiProxy + init by EOA = in two transactions https://rinkeby.etherscan.io/address/0x023b8c95230D20b8D961E1A3D865329ed68D948a

Also a proof - tests separate transactions too. Also a proof - initiateHomeFi() in HomeFiProxy deploys contracts without init()'s

Where everything is ok - Project.sol - Project both deployed initialized by ProjectFactory in one transaction.

Tools Used

Etherscan, Hardhat

Recommended Mitigation Steps

Options:

1. Make deployments+init in one transaction via multisig wallet supporting wrapping transactions in calls.
2. Change HomeFiProxy to deploy all contracts separately. The first transaction for deploy+init of ProjectFactory. The second transaction for deploy+init of Community.sol. An so one.
3. Place all transactions to flashbots, in one bundle.

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L232

Vulnerability details

Impact

Inputed _hash for HomeFi.sol is the URI for NFTs. Contracts do not check duplicates and do not check is it correct or not. It is not dangerous within smart-contracts (as NFT are not used much here), but it can be important together with frontend or backend. Hashes will not be unique.

Proof of Concept

createProject() https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L210-L232 mintNFT() https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/HomeFi.sol#L284-L297

Tools Used

Hardhat

Recommended Mitigation Steps

Check is it necessary for NFT hashes to be unique - if so, write checks.