QA Report

[code423n4]

Condition in receive function can be bypassed with self-destruct of another contract

Details: The logic in L436-L438 implies that the contract should only receive ether if isClaimingBribes is true. However, this check can be bypassed by deploying a contract (say, Attacker) and setting up the address of MyStrategy contract as the destination of a selfdestruct in the Attacker contract — for more information and otherway to bypass the require of L437, see this link.

Impact: Informational (could possibly break internal calculations of the protocol though)

Re-entrancy guard upgradeable contract is not initialized

Details: As stated in OpenZeppelin docs, “when writing an initializer, you need to take special care to manually call the initializers of all parent contracts”. However, the initializer of ReentrancyGuardUpgradeable is not called.

Mitigation: Ensure that all necessary functions are inherited from the upgradeable contracts.

Impact: Code QA

TODOs are left in comments

Details: In L284 and L422 of MyStrategy.sol there are comments with TODOs. These should be resolved and removed from the code before deployment.

Mitigation: Check the TODOs and fix/remove them.

Impact: Code QA

Usage of deprecated function safeApprove

Details: In L65-68 of MyStrategy.sol the function safeApprove from OpenZeppelin contracts are used, however these functions have been deprecated according to OpenZeppelin 3.x docs (note that MyStrategy.sol correctly use OpenZeppelin 3.4.0).

Impact: Code QA

Alert developers that OpenZeppelin contract cannot be bumped to 4.x

Note to judges: I think this issue is out-of-scope, but worthy to inform anyway 🙂

Details: According to brownie config file, the contract MyStrategy.sol imports version 3.4.0 of OpenZeppelin’s SafeMath, and this is the recommend version to use with Solidity 0.6.12.

Unware developers, however, may want to bump OpenZeppelin version to the lastest one, and running brownie compile will compile the contract without errors (at least for 4.6.0). However, as alerted by the comments in L6-8, recent versions of SafeMath should only be used with Solidity 0.8 or later, because it relies on the compiler's built in overflow checks. This implies that checks of overflow/underflow will not be used, and this could be further exploited in other attacks.

This could also be particularly dangerous in the scenario wherein a developer does this bumping while writing his own MyStrategy contract, since he will probably use the SafeMath functions assuming that underflow/overflow checks are being used in his code.

Mitigation: Consider adding a comment in brownie config file alerting the users that OpenZeppelin version should not be bumped.

Impact: Informational (probably out-of-scope)

[GalloDaSballo]

Condition in receive function can be bypassed with self-destruct of another contract

My conclusion from this is to add a sweep for ETH, that said who's going to give us free money? -> entreprenerd.eth

Re-entrancy guard upgradeable contract is not initialized

Agree that we should and agree with severity as it's just a gas cost

Usage of deprecated function safeApprove

I'm pretty sure we're using it the correct way (one time, from 0 to X)

Alert developers that OpenZeppelin contract cannot be bumped to 4.x

Agree with the heads up and believe this is the correct severity for the other "OZ Initializer Vulnerability hehe XD"