Canto v2 contest - Sm4rty's results

1. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Instances:

Comptroller.sol: 1385

BaseV1-periphery.sol: #L291 #L437 #L309 #L485

AccountantDelegate.sol#L93 TreasuryDelegate.sol#L86 CNote.sol#L31 WETH.sol#L31 Reservoir.sol#64 CNote.sol#L77

lending-market-v2/contracts/Comptroller.sol:1385:            comp.transfer(user, amount);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:291:        assert(wcanto.transfer(pair, amountCANTO));
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:437:        assert(wcanto.transfer(pairFor(routes[0].from, routes[0].to, routes[0].stable), amounts[0]));
lending-market-v2/contracts/Accountant/AccountantDelegate.sol:93:               bool success = cnote.transfer(treasury, amtToSweep);
lending-market-v2/contracts/Treasury/TreasuryDelegate.sol:86:            note.transfer(recipient, amount);
lending-market-v2/contracts/CNote.sol:131:        token.transfer(to, amount);
lending-market-v2/contracts/WETH.sol:31:        payable(msg.sender).transfer(wamount); // rentrant attack must be less than 2300 gas
lending-market-v2/contracts/Reservoir.sol:64:    token_.transfer(target_, toDrip_);


lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:309:        require(IBaseV1Pair(pair).transferFrom(msg.sender, pair, liquidity)); // send liquidity to pair
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:485:        tokenCon.transferFrom(from, to, value);
lending-market-v2/contracts/CNote.sol:77:        token.transferFrom(from, address(this), amount); //allowance set before

Reference:

This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

---

2. USE SAFEERC20.SAFEAPPROVE INSTEAD OF APPROVE

This is probably an oversight since SafeERC20 was imported and safeTransfer() was used for ERC20 token transfers. Nevertheless, note that approve() will fail for certain token implementations that do not return a boolean value (). Hence it is recommend to use safeApprove().

Instances:

AccountantDelegate.sol:41

AccountantDelegate.sol:41:               note.approve(cnoteAddress_, type(uint).max);

Recommended Mitigation Steps

Update to note.safeapprove(cnoteAddress_, type(uint).max); in the function.

3. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

Instances

Comptroller.sol: 1237 Comptroller.sol: 1276 Reservoir.sol: 1237

lending-market-v2/contracts/Comptroller.sol:1237:        // TODO: Don't distribute supplier COMP if the user is not in the supplier market.
lending-market-v2/contracts/Comptroller.sol:1276:        // TODO: Don't distribute supplier COMP if the user is not in the borrower market.
lending-market-v2/contracts/Reservoir.sol:49:    uint reservoirBalance_ = token_.balanceOf(address(this)); // TODO: Verify this is a static call

---

4. _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren’t lost if they’re minted to contracts that cannot transfer them back out.

Instances

Stableswap/BaseV1-core.sol:252 Stableswap/BaseV1-core.sol:257 Stableswap/BaseV1-core.sol:396 ending-market-v2/contracts/Note.sol:14

lending-market-v2/contracts/Stableswap/BaseV1-core.sol:252:            _mint(address(0), MINIMUM_LIQUIDITY); // permanently lock the first MINIMUM_LIQUIDITY tokens
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:257:        _mint(to, liquidity);
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:396:    function _mint(address dst, uint amount) internal {
lending-market-v2/contracts/Note.sol:14:        _mint(accountantDelegator, type(uint).max);

Recommendations:

Use _safeMint() instead of _mint().

1. !=0 instead of >0 for UINT

> 0 is less efficient than != 0 for unsigned integers (with proof) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

Instances

GovernorAlpha.sol:228 BaseV1-periphery.sol:122 BaseV1-periphery.sol:123 BaseV1-periphery.sol:474 BaseV1-periphery.sol:481 BaseV1-core.sol:256 BaseV1-core.sol:275 BaseV1-core.sol:289 BaseV1-core.sol:306 BaseV1-core.sol:468 CToken.sol:37

lending-market-v2/contracts/Governance/GovernorAlpha.sol:228:        require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id");
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:122:        require(amountA > 0, "BaseV1Router: INSUFFICIENT_AMOUNT");
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:123:        require(reserveA > 0 && reserveB > 0, "BaseV1Router: INSUFFICIENT_LIQUIDITY");
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:474:        require(token.code.length > 0);
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:481:        require(token.code.length > 0, "token code length failure");
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:256:        require(liquidity > 0, 'ILM');
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:275:        require(amount0 > 0 && amount1 > 0, 'ILB'); 
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:289:        require(amount0Out > 0 || amount1Out > 0, 'IOA');
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:306:        require(amount0In > 0 || amount1In > 0, 'IIA'); 
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:468:        require(token.code.length > 0);
lending-market-v2/contracts/CToken.sol:37:        require(initialExchangeRateMantissa > 0, "initial exchange rate must be greater than zero.");

Reference:

https://twitter.com/gzeon/status/1485428085885640706

Remediation:

I suggest changing > 0 with != 0. Also, please enable the Optimizer.

---

2. Variables: No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

We can use uint number; instead of uint number = 0;

Instance Includes:

Comp.sol:207 BaseV1-periphery.sol:176 BaseV1-core.sol:48 BaseV1-core.sol:72 BaseV1-core.sol:226 BaseV1-core.sol:227 CToken.sol:82

lending-market-v2/contracts/Governance/Comp.sol:207:        uint32 lower = 0;
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:176:        uint _totalSupply = 0;
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:48:    uint public totalSupply = 0;
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:72:    uint constant periodSize = 0;
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:226:        uint nextIndex = 0;
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:227:        uint index = 0;
lending-market-v2/contracts/CToken.sol:82:        uint startingAllowance = 0;

Recommendation:

I suggest removing explicit initializations for default values.

---

3. An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Instances:

GovernorBravoDelegate.sol:66 GovernorBravoDelegate.sol:66 GovernorAlpha.sol:181 GovernorAlpha.sol:197 GovernorAlpha.sol:211 Comptroller.sol: 742 Comptroller.sol: 964 Comptroller.sol: 1111 Comptroller.sol: 1352 Comptroller.sol: 1358 Comptroller.sol: 1364 Comptroller.sol: 1369 BaseV1-periphery.sol:154 BaseV1-periphery.sol:380 BaseV1-core.sol:210

lending-market-v2/contracts/Governance/GovernorBravoDelegate.sol:66:        for (uint i = 0; i < newProposal.targets.length; i++) {
lending-market-v2/contracts/Governance/GovernorBravoDelegate.sol:88:        for (uint i = 0; i < proposal.targets.length; i++) {
lending-market-v2/contracts/Governance/GovernorAlpha.sol:181:        for (uint i = 0; i < proposal.targets.length; i++) {
lending-market-v2/contracts/Governance/GovernorAlpha.sol:197:        for (uint i = 0; i < proposal.targets.length; i++) {
lending-market-v2/contracts/Governance/GovernorAlpha.sol:211:        for (uint i = 0; i < proposal.targets.length; i++) {
lending-market-v2/contracts/Comptroller.sol:742:        for (uint i = 0; i < assets.length; i++) {
lending-market-v2/contracts/Comptroller.sol:964:        for (uint i = 0; i < allMarkets.length; i ++) {
lending-market-v2/contracts/Comptroller.sol:1111:        for (uint i = 0; i < affectedUsers.length; ++i) {
lending-market-v2/contracts/Comptroller.sol:1352:        for (uint i = 0; i < cTokens.length; i++) {
lending-market-v2/contracts/Comptroller.sol:1358:                for (uint j = 0; j < holders.length; j++) {
lending-market-v2/contracts/Comptroller.sol:1364:                for (uint j = 0; j < holders.length; j++) {
lending-market-v2/contracts/Comptroller.sol:1369:        for (uint j = 0; j < holders.length; j++) {
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:154:        for (uint i = 0; i < routes.length; i++) {
lending-market-v2/contracts/Stableswap/BaseV1-periphery.sol:380:        for (uint i = 0; i < routes.length; i++) {
lending-market-v2/contracts/Stableswap/BaseV1-core.sol:210:        for (uint i = 0; i < _prices.length; i++) {

Remediation:

Here, I suggest storing the array’s length in a variable before the for-loop, and use it instead.