search

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

2 stars 0 forks source link

QA Report #220

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Concur Finance QA Report

Unless otherwise noted, manual auditing and testing were done using Visual Studio Code and Remix. The audit was done from February 3-9, 2022 by ye0lde through code4rena.

Overall, I found the code to be clear to follow and read. More in-code documentation of contracts and functions would be useful to help others understand the code. I also recommend the team improve the supporting documentation to give a better overall understanding of the protocol. There was no test suite provided. Including comprehensive tests that tackle edge scenarios and provide coverage and gas reports would be useful.

Findings

L-1 - Unbounded iteration in massUpdatePools (MasterChef.sol)

Impact

The massUpdatePools function iterates over all pool elements and updates them by calling updatePool. It can fail if the number of pools gets too large and the transaction consumes more gas than the block limit.

Proof of Concept

massUpdatePools is here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/MasterChef.sol#L126-L132

    // Update reward variables for all pools. Be careful of gas spending!
    function massUpdatePools() public {
        uint length = poolInfo.length;
        for (uint _pid = 0; _pid < length; ++_pid) {
            updatePool(_pid);
        }
    }

Recommended Mitigation Steps

Consider adding startIndex, count parameters that allow processing the array in sections and possibly enforcing a limit on count.

L-2 - Unsafe typecasts in _calcRewardIntegral (ConvexStakingWrapper.sol.sol)

Impact

_calcRewardIntegral performs several unsafe typecasts to uint128 without checking if the values actually fit into 128 bits. These typecasts don't seem to directly lead to exploits but safe type casts should still be implemented for additional security.

Proof of Concept

The typecasts are here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L187 https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/ConvexStakingWrapper.sol#L203

Recommended Mitigation Steps

I suggest using the SafeCast library as is done in Masterchef to do these casts.

NC-1 - Incorrect revert message in setRewardsDistribution (StakingRewards.sol)

Impact

Misleading reporting

Proof of Concept

The incorrect message which appears to be copied from line #181 is here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/StakingRewards.sol#L193

Recommended Mitigation Steps

I suggest changing the message to: Previous rewards period must be complete before changing the rewards distribution address

NC-2 - No event defined or emitted for donate (Shelter.sol)

Impact

State variables are changed and a transfer is being performed similarly to activate, deactivate and withdraw but no explicit "donate" event is emitted for function donate.

Proof of Concept

donate and the similar functions are here: https://github.com/code-423n4/2022-02-concur/blob/72b5216bfeaa7c52983060ebfc56e72e0aa8e3b0/contracts/Shelter.sol#L32-L57

Recommended Mitigation Steps

Consider adding a donate event

GalloDaSballo commented 2 years ago

L-1 - Unbounded iteration in massUpdatePools (MasterChef.sol) Valid but don't think Low is legitimate especially given that the function is not used and that you'd need thousands of pool to actually trigger it

L-2 - Unsafe typecasts in _calcRewardIntegral (ConvexStakingWrapper.sol.sol) Agree

Other 2 findings are non-critical

GalloDaSballo commented 2 years ago

1+++