Fractional v2 contest - pashov's results

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L63

Vulnerability details

Proof of concept: The execute() function in Vault.sol allows the owner address to call it any time with any parameters, and in it’s code it does delegatecall to the function’s target argument with the function’s data argument. This basically allows the owner to execute any code, any time in the context of Vault.sol

Impact: This is a bit of a centralisation issue, where one address has too much power. This address can be malicious or hacked and execute code that results in change in the mapping from function selector to plugin addresses for example.

Recommended Mitigation Steps: Put owner calls to execute() behind a timelock, so users can have time to withdraw funds or do any actions if they feel that the call is not safe for them.

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/FERC1155.sol#L217

Vulnerability details

Proof of concept: The controller of the contract can set any royalty percentage for any ERC1155 token, but the setting is not limited, so he can set it to 100% which will result in all the funds of a trade going to the royaltyAddress. He can also set the royaltyAddressto be one that is controlled by him, effectively getting 100% of all funds which users use on secondary sales

Impact: If controller is malicious or hacked, he can basically rug all secondary sales and steal the value from them completely.

Recommended Mitigation Steps: Implement an upper limit for royaltyPercent, for example make it so the maximum percentage is 5%. Also make it so that the owner of the token can set the royaltyAddress, otherwise the controller can always set it to his address and steal all royalties

Lines of code

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L172 https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L325

Vulnerability details

Impact:

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

1. The claimer smart contract does not implement a payable function.
2. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.

Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Recommended Mitigation Steps: Use **** call() instead of transfer()

Checking if a contract exists before delegatecall is not needed since Solidity 0.8.0

Scope: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L115

The code has a check and reverts if the target for delegateCall does not have any code (it’s an EOA). Since Solidity 0.8.10 this is not needed

• Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist.

Open TODO in codebase

Scope: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/MerkleBase.sol#L24

Resolve hanging TODO before deploying to mainnet.

WETH address variable should not be payable

Scope: https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/utils/SafeSend.sol#L11

Do not use address payable for WETH, ether shouldn’t be directly sent to WETH contract ever. Use just address

Wrong comment

Scope:https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L32

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Buyout.sol#L53

https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/modules/Migration.sol#L63

The comment above the function is saying /// @dev Callback for receiving ether when the calldata is empty but this is not a “callback”. The intention might have been to write “fallback” which is still partly correct. Replace callback with function