search

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

2 stars 0 forks source link

QA Report #217

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Overall

There are 3 Non-critical issues and 3 Low severity issues found.

[L1] USDMPegRecovery.sol implementation is not spec compliant

Per to README:

https://github.com/code-423n4/2022-02-concur/tree/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0#-usdmpegrecovery

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

In the implementation:

The actual threshold is 4m. Only 1/10 of the expected 40m;

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/USDMPegRecovery.sol#L90-L108

function deposit(Liquidity calldata _deposits) external {
    Liquidity memory total = totalLiquidity;
    Liquidity memory user = userLiquidity[msg.sender];
    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;
    }
    totalLiquidity = total;
    userLiquidity[msg.sender] = user;
    emit Deposit(msg.sender, _deposits);
}

Recommendation

Consider adding a new constant:

uint128 private constant MIN_USDM_AMOUNT_WHEN_DEPOSIT_POOL3 = 40e24;

And change:

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

to:

require(totalLiquidity.usdm >= MIN_USDM_AMOUNT_WHEN_DEPOSIT_POOL3, "usdm low");

[L2] Certain methods should be disabled when paused

Based on common practices, pause is used for preventing loss of value in emergency situations, which should disable certain methods that can change essential states and transfer funds.

Therefore, we believe the following methods should be disabled when the contract is paused.

Recommendation

Consider adding whenNotPaused to the methods above, or if the intention is to pause deposit only, consider adding a new storage variable called depositPaused.

[N3] Code Style: constants should be named in all caps

Here are some examples that the code style does not follow the best practices:

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L28-L36

//constants/immutables
address public constant convexBooster =
    address(0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
address public constant crv =
    address(0xD533a949740bb3306d119CC777fa900bA034cd52);
address public constant cvx =
    address(0x4e3FBD56CD56c3e72c1403e103b45Db9da5B9D2B);

uint256 public constant CRV_INDEX = 0;

[L4] Unchecked return value for token.transfer call

It is usually good to add a require-statement that checks the return value or to use something like safeTransfer; unless one is sure the given token reverts in case of a failure.

Instances include:

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

IERC20(reward.token).transfer(address(claimContract), d_reward);

Recommendation

Consider adding a require-statement or using safeTransfer of SafeERC20.

[N5] Constants are not explicitly declared

It's a best practice to use constant variables rather than literal values to make the code easier to understand and maintain.

Consider defining a constant variable for the literal value used and giving it a clear and self-explanatory name.

Instances include:

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L67-L71

return
    rewardPerTokenStored +
    (((lastTimeRewardApplicable() - lastUpdateTime) *
        rewardRate *
        1e18) / _totalSupply);

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L75-L79

return
    (_balances[account] *
        (rewardPerToken() - userRewardPerTokenPaid[account])) /
    1e18 +
    rewards[account];

Consider changing 1e18 to REWARD_PER_TOKEN_MULTIPLIER constant.

[N6] Interface IRewardStaking mismatch implementations

https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/external/ConvexInterfaces.sol#L12-L34

interface IRewardStaking {
    struct PoolInfo {
        address lptoken;
        address token;
        address gauge;
        address crvRewards;
        address stash;
        bool shutdown;
    }

    function poolInfo(uint256) external view returns(PoolInfo memory);
    function stakeFor(address, uint256) external;
    function stake( uint256) external;
    function withdraw(uint256 amount, bool claim) external;
    function withdrawAndUnwrap(uint256 amount, bool claim) external;
    function earned(address account) external view returns (uint256);
    function getReward() external;
    function getReward(address _account, bool _claimExtras) external;
    function extraRewardsLength() external view returns (uint256);
    function extraRewards(uint256 _pid) external view returns (address);
    function rewardToken() external view returns (address);
    function balanceOf(address _account) external view returns (uint256);
}

The IRewardStaking interface above is:

A misleading interface can cause issues like [WP-H2].

Recommendation

Consider changing the type declaring for addresses.

For example:

-

    mapping(uint256 => address) public convexPool;
can be changed to:

```solidity
mapping(uint256 => IRewardStaking) public convexPool;
```

`IRewardStaking` should be: https://github.com/convex-eth/platform/blob/main/contracts/contracts/interfaces/IRewardStaking.sol

-

    address public constant convexBooster = address(0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
can be changed to:

```solidity
IConvexBooster public constant convexBooster = address(0xF403C135812408BFbE8713b5A23a04b3D48AAE31);
```

IConvexBooster should be defined based on: https://github.com/convex-eth/platform/blob/main/contracts/contracts/Booster.sol
GalloDaSballo commented 2 years ago

L1 Agree with "typo" as well as with avoiding magic numbers This is the one instance of Code != Comment that I've bumped to Low

L2 Personally disagree as withdrawal could be left always open (up to sponsor implementation)

N3 Agree with convention

L4 Agree with need to check

L5 Agree with usage of constants over magic numbers

N6 Agree on oddity I belive the interface combines the booster with the BaseRewardPool

GalloDaSballo commented 2 years ago

3++