code-423n4 / 2022-02-concur-findings

2 stars 0 forks source link

QA Report #171

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1 Lock pragmas to specific compiler version. Not ^0.8.11. e.g pragma solidity 0.8.11; 2 Unchecked emptyAddress for immutable variable in the constructor. https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L79-L80

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L69-L72

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L28-L30

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol#L37-L47

require(_input != address(0), “zero address”);

ConvexStakingWrapper.sol

3 Use safeTransfer instead of transfer

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L179

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L182

USDMPegRecovery.sol

4 Use safeApprove

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L79-L80

MasterChef.sol

5 Unchecked inputs(_startBlock, _endBlock) in constructor. _startBlock must be earlier than _endBlock

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L59-L71

require(_startBlock < _endBlock, “start must be ealier”);

6 Delete if (_amount > 0) in deposit and withdraw because _amount is already checked in stake/deposit and withdraw of StakingRewards.sol or ConvexStakingWrapper.sol.

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L169

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L194

7 Same logic and code in pendingConcur, deposit and withdraw. You can create another internal function _pendingConcur and delete this code from these functions.

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L123 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L163 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/MasterChef.sol#L190

For example

function _pendingConcur(uint128 _userAmount, uint _accConcurPerShare, uint _multiplier, uint128 _userRewardDebt) internal returns (uint) { return _userAmount * _accConcurPerShare / _multipiler - userRewardDebt }

StakingRewards.sol

8 Unchecked emptyAddress for immutable variable in the constructor

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/StakingRewards.sol#L37-L47

require(_masterChef != address(0), “zero address”);

GalloDaSballo commented 2 years ago

1 Lock pragmas to specific compiler version. Not ^0.8.11. e.g pragma solidity 0.8.11; Disagree that it matters

2 Unchecked emptyAddress for immutable variable in the constructor. Valid per current auditing standards

3 Use safeTransfer instead of transfer Agree -> Means will ignore #55

5 Unchecked inputs(_startBlock, _endBlock) in constructor. _startBlock must be earlier than _endBlock Agree

6 Delete if (_amount > 0) in deposit and withdraw because _amount is already checked in stake/deposit and withdraw of StakingRewards.sol or ConvexStakingWrapper.sol. Even if valid, it's just gas

7 Same logic and code in pendingConcur, deposit and withdraw. You can create another internal function _pendingConcur and delete this code from these functions. Gas

8 Unchecked emptyAddress for immutable variable in the constructor Agree

All in all 4 valid findings and rest is mostly preference / gas