search

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

2 stars 0 forks source link

QA Report #174

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Non Critical and Low Risk Issues

---------------------------------------------------------------------------

1 Low-Risk - Divergence between code and documentation

In documentation:

Once 40m USDM is deposited, 3Crv side of the contract starts accepting deposits.

But in code:

require(totalLiquidity.usdm > 4000000e18, "usdm low");

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

Tools Used

Manual review

Recommended Mitigation Steps

Update either one

---------------------------------------------------------------------------

2 Low-Risk - Divergence between code and documentation

In documentation:

This contract holds the rewards that has to go to Convex LP stakers. 80 % of all rewards will be pushed to stakers and user can claim tokens here.

But in code there is no 80% computation

Tools Used

Manual review

Recommended Mitigation Steps

Update either code or docs

---------------------------------------------------------------------------

1 Non-Critical - Prefer constant variables instead of hardcoded constant values

require(totalLiquidity.usdm > 4000000e18, "usdm low");

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

Tools Used

Manual review

Recommended Mitigation Steps

Rewrite to constant variable

---------------------------------------------------------------------------

2 Non-Critical - Roles can simplified with AccessControl

RewardNotifierRole in ConcurRewardPool Owner, Depositor roles in MasterChef Owner, Guardian, KPI_ORACLE roles in USDMPegRecovery

Tools Used

Manual review

Recommended Mitigation Steps

Consider using AccessControl

---------------------------------------------------------------------------

3 Non-Critical - Consider adding doc comments. Consider adding examples

There are no comments or there are insufficient comments and there are no usage examples:

IShelter.sol

IShelterClient.sol

IConcurRewardClaim.sol

Iauction.sol

EasySign.sol

VoteProxy.sol

Tools Used

Manual review

Recommended Mitigation Steps

Consider making your codebase developer/user/auditor friendly by adding comments and examples

---------------------------------------------------------------------------

4 Non-Critical - MasterChef contract is a copy-pasted

MasterChef.sol has some functions/events not used. It is usually a bad smell

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing your own Staking contract or at least copy functionality that you are certain about

---------------------------------------------------------------------------

5 Non-Critical - Inconsistency

In: https://github.com/code-423n4/2022-02-concur/blob/main/contracts/USDMPegRecovery.sol#L51 there is explicit Ownable() constructor invocation. It's not needed and other contracts don't call it explicitly

Tools Used

Manual review

Recommended Mitigation Steps

Either be consistent or remove explicit call

---------------------------------------------------------------------------

2 - Accessing array length in for loop can be optimized by caching in local variable

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider caching length before the for loop.

uint256 length = array.length
for (uint256 i; i < length; ++i) {
    ...
}

---------------------------------------------------------------------------

3 - Ownable contract not used.

Inheriting from Ownable cost unnecessary deployment cost (storage variables and functions) and some runtime cost (choosing function selectors)

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove Ownable contract from inheritance and it's import

---------------------------------------------------------------------------

4 - SafeMath not used.

Using SafeMath and it's functions with compiler >=0.8 cost unnecessary STATICCALLs to library without any benefit. Imo, it also makes code less readable.

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove SafeMath import and it's usage

---------------------------------------------------------------------------

5 - Event not used.

Event is not emitted. It costs unnecessary gas and makes code less understandable

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove EmergencyWithdraw event

---------------------------------------------------------------------------

6 - Function can be made private or inlined by hand

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

// Return reward multiplier over the given _from to _to block.
function getMultiplier(uint _from, uint _to) public pure returns (uint) {
    return _to.sub(_from);
}

This is basically doing _to - _from (SafeMAth doesn't add extra logic see above)

Tools Used

Manual review

Recommended Mitigation Steps

Consider inlining it or at least marking this function private so it can be inlined by compiler.

---------------------------------------------------------------------------

7 - Unused functions

There are few unused functions. For example massUpdate()

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider removing unused functions to save deployment gas and some minor runtime gas (choosing function selector)

---------------------------------------------------------------------------

8 - Long revert strings

Strings longer than 32 cost additional storage and access during revert

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

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Consider shortening revert strings or use custom errors

---------------------------------------------------------------------------

9 - Fail fast - reorder conditions to possibly fail faster

Second if statement's body contains revert expression which is highly to revert. Reordering it to fail fast can save users cost

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

Tools Used

Manual review

Recommended Mitigation Steps

Reorder this:

if(_deposits.usdm > 0) {
    usdm.safeTransferFrom(msg.sender, address(this), uint256(_deposits.usdm));
    total.usdm += _deposits.usdm;
    user.usdm += _deposits.usdm;
}

if(_deposits.pool3 > 0) {
    require(totalLiquidity.usdm > 4000000e18, "usdm low");
    pool3.safeTransferFrom(msg.sender, address(this), uint256(_deposits.pool3));
    total.pool3 += _deposits.pool3;
    user.pool3 += _deposits.pool3;
}

to this:

if(_deposits.pool3 > 0) {
    require(totalLiquidity.usdm > 4000000e18, "usdm low");
    pool3.safeTransferFrom(msg.sender, address(this), uint256(_deposits.pool3));
    total.pool3 += _deposits.pool3;
    user.pool3 += _deposits.pool3;
}

if(_deposits.usdm > 0) {
    usdm.safeTransferFrom(msg.sender, address(this), uint256(_deposits.usdm));
    total.usdm += _deposits.usdm;
    user.usdm += _deposits.usdm;
}

---------------------------------------------------------------------------

10 - Using pre-increment (++i) instead of post-increment (i++) can save gas

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Change i++ to ++i

---------------------------------------------------------------------------

11 - Assigning default values costs unnecessary gas.

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

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

Tools Used

Manual review

Recommended Mitigation Steps

Remove default value assignments.

---------------------------------------------------------------------------

GalloDaSballo commented 2 years ago

1 Low-Risk - Divergence between code and documentation Agree

2 Low-Risk - Divergence between code and documentation Non-critical

1 Non-Critical - Prefer constant variables instead of hardcoded constant values Agree

2 Non-Critical - Roles can simplified with AccessControl Would like to see actual detailed explanation instead of throwaway comment

3 Non-Critical - Consider adding doc comments. Consider adding examples Disagree

4 Non-Critical - MasterChef contract is a copy-pasted This is not a professional comment

Everything else is gas which should be submitted as gas report

I genuinely feel that some of these blank statement need more backing. If you're going to blankly tell someone they are wrong at least spend the extra time showing how or the findings become unactionable.

As in, detail what code is wrong, and how to rewrite it to improve it

GalloDaSballo commented 2 years ago

1++