FactoryDAO contest - rajatbeladiya's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L282 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L169

Vulnerability details

Impact

All rewards calculations are referring to 18 decimals token. If reward token will have less than 18 decimals, calculations will be wrong and user will receive less tokens than he should be receiving.

Proof of Concept

If deposit token or reward token has 9 decimals

• getMaximumRewards() will return almost zero and pool will created with 0 reward token transfer.

• getRewards(poolId, receiptId) which is called from withdraw() function will return less reward to users compare to user should get and owner wanted to distribute.

Tools Used

Manual Analysis

Recommended Mitigation Steps

calculate rewards with wei only.

Events are missing

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L314

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L261

• SetGlobalTax
• withdrawTaxes

Zero address check validation missing

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L75

• check zero address validation for _globalBeneficiary, it could not change once it deployed.

Wrap transfer ERC20 transaction into require

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L198

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L230

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L233

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L252

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L269

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L144

• removing a bool success will save some gas and best practice is to wrap transfer ERC20 transactions to require directly.

Use

require(IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount), 'Token transfer failed');

Instead of

bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
require(success, 'Token transfer failed');

Remove unnecessary uint tax

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L266-L270

Use

success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary,  taxes[poolId][i]);
taxes[poolId][i] = 0;

Instead of

uint tax = taxes[poolId][i];
taxes[poolId][i] = 0;
success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);