search

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

2 stars 0 forks source link

QA Report #266

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Missing 0 Address Check

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

    constructor(address _notifier) {
        rewardNotifier = _notifier;
    }

The rewardNotifier is a crucial parameter that should be zero address checked.

Missing input validation for _amount in deposit and withdraw

Severity: Low https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L236 https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L262

The _amount parameter to the withdraw and deposit functions should be checked not to be zero. Otherwise, the following if statements:

        if (_amount > 0) {

Will not trigger, allowing an user to accidentially reset their deposit or withdraw request epoch and lock funds for another cycle. This can also lead to the spam of events at the end of these functions which may cause issues with logging.

Use of msg.sender and _msgSender can result in wasted gas

Severity: Low/Gas Depositors may look the the body of the MasterChef.sol#withdraw and MasterChef.sol#deposit function and note that it is possible to use meta transactions to interact with this contract due to the user of _msgSender. However, in the onlyDepositor modifier, msg.sender is used which would be the relayer, not the meta transaction originator. This may result in user confusion and frustration or wasted gas if the meta transaction functionality was not necessary.

Public function should be External

Severity: Gas Since the following functions are not called from within their own contract, the visibility quantifier shoudl be set to external to save on gas. https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L93

Cache calls to External Contracts

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L93-L99

    function addRewards(uint256 _pid) public {
        address mainPool = IRewardStaking(convexBooster)
            .poolInfo(_pid)
            .crvRewards;
        if (rewards[_pid].length == 0) {
            pids[IRewardStaking(convexBooster).poolInfo(_pid).lptoken] = _pid;
            convexPool[_pid] = mainPool;

On ConvexStakingWrapper.sol#L94-85 there is a call made to RewardStaking(convexBooster).poolInfo(_pid). This makes an external call to the convexBooster contract. This same call is then performed just a couple lines down on line 98. Caching the result of this would reduce gas usage significantly for this function.

Avoid Interactions with External Contracts Where Possible

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L209-L222

    function _checkpoint(uint256 _pid, address _account) internal {
        //if shutdown, no longer checkpoint in case there are problems
        if (paused()) return;

        uint256 supply = _getTotalSupply(_pid);
        uint256 depositedBalance = _getDepositedBalance(_pid, _account);

        IRewardStaking(convexPool[_pid]).getReward(address(this), true);

        uint256 rewardCount = rewards[_pid].length;
        for (uint256 i = 0; i < rewardCount; i++) {
            _calcRewardIntegral(_pid, i, _account, depositedBalance, supply);
        }
    }

In the _checkpoint function, if a user does not have any depositedBalance, then they will not have any rewards that need to be issued. This is true since when a user calls the withdraw function (the only function which decreases the user's deposited balance) that function will perform a _checkpoint call which will transfer the user's rewards to be claimed. As such, the deposit amount cannot ever be 0 and there still be rewards left to be claimed.

As such, adding an if statement check whether there is a depositedBalance before reaching out to the convexPool will save the initial gas cost when first creating a deposit, which will decrease the friction of using Concur for the first time.

Unchecked Math

Severity: Gas https://github.com/code-423n4/2022-02-concur/blob/main/contracts/ConvexStakingWrapper.sol#L261 It is possible to put ConvexStakingWrapper.sol#L261 inside the unchecked directive. This is due to the fact that on line 259 we see that the request.amount much be greater-than or equal to the requested _amount. This request.amount is limited by the deposits[_pid][msg.sender].amount in the requestWithdraw function. The only way for the deposits[_pid][msg.sender].amount to decrease is via the withdraw function. This withdraw function also deletes the withdrawRequest on completion and is nonReentrant. As a result, it is impossible for _amount to be greater than deposits[_pid][msg.sender].amount.

However, if you do decide to make this code optimization, I'd recommend commenting around it explaining the above as it does add complexity to the code and a modification elsewhere could turn this into a vulnerability.

External Calls in Modifiers

https://github.com/code-423n4/2022-02-concur/blob/229b6188a2c5867f2d0cb4579d0a0d49516da252/contracts/ConvexStakingWrapper.sol#L73-L79 The whenNotInShelter modifier makes two external calls to verify whether a _pid is in the shelter or not. This costs a tremendous amount of gas. I'd recommend simply creating a mapping for _pid to inShelter and modifying it as required. This will save a lot of gas every time a function using the whenNotInShelter modifier is used.

GalloDaSballo commented 2 years ago

Dup submission