Canto Invitational - erebus's results

Lines of code

https://github.com/code-423n4/2024-01-canto/blob/5e0d6f1f981993f83d0db862bcf1b2a49bb6ff50/src/LendingLedger.sol#L67-L70

Vulnerability details

Impact

When calculating the amount of CANTO per share in update_market, dividing by 1e18 in cantoReward and multiplying by the same value in accCantoPerShare rounds down the final value, making the amount of rewards users will receive be less than expected.

Proof of Concept

It's well known that Solidity rounds down when doing an integer division, and because of that, it is always recommended to multiply before dividing to avoid that precision loss. However, if we go to

LendingLedger, function update_market

    function update_market(address _market) public {
        require(lendingMarketWhitelist[_market], "Market not whitelisted");
        MarketInfo storage market = marketInfo[_market];
        if (block.number > market.lastRewardBlock) {
            uint256 marketSupply = lendingMarketTotalBalance[_market];
            if (marketSupply > 0) {
                uint256 i = market.lastRewardBlock;
                while (i < block.number) {
                    uint256 epoch = (i / BLOCK_EPOCH) * BLOCK_EPOCH; // Rewards and voting weights are aligned on a weekly basis
                    uint256 nextEpoch = i + BLOCK_EPOCH;
                    uint256 blockDelta = Math.min(nextEpoch, block.number) - i;
                    uint256 cantoReward = (blockDelta *
                        cantoPerBlock[epoch] *
                        gaugeController.gauge_relative_weight_write(_market, epoch)) / 1e18;
                    market.accCantoPerShare += uint128((cantoReward * 1e18) / marketSupply);
                    market.secRewardsPerShare += uint128((blockDelta * 1e18) / marketSupply); // TODO: Scaling
                    i += blockDelta;
                }
            }
            market.lastRewardBlock = uint64(block.number);
        }
    }

and we expand the maths behind accCantoPerShare and cantoReward:

                    uint256 cantoReward = (blockDelta *
                        cantoPerBlock[epoch] *
                        gaugeController.gauge_relative_weight_write(_market, epoch)) / 1e18;
                    market.accCantoPerShare += uint128((cantoReward * 1e18) / marketSupply);

like follows:

$(cantoReward * 1e18) \ / \ marketSupply$

$(((blockDelta * cantoPerBlock[epoch] * gaugeController.gauge\_relative\_weight\_write(\_market, epoch)) \ / \ 1e18) \ * \ 1e18) \ / \ marketSupply$

$((stuff \ / \ 1e18) \ * \ 1e18) \ / \ marketSupply$

we see there is a hidden division before a multiplication that is rounding down the whole expression. This is bad as the precision loss can be significant, which leads to the given market having less rewards to offer to its users. Run the next test inside a foundry project to see such a divergence in the precision if we multiply before dividing:

    // @audit the assumes are to avoid under/overflows errors
    function testPOC(uint256 x) external pure {
        vm.assume(x < type(uint256).max / 1e18);
        vm.assume(x > 1e18);
        console2.log("(x / 1e18) * 1e18", (x / 1e18) * 1e18);
        console2.log("(x * 1e18) / 1e18", (x * 1e18) / 1e18);
    }

Some examples:

  [7725] POC::testPOC(1164518589284217277370 [1.164e21]) 
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x / 1e18) * 1e18, 1164000000000000000000 [1.164e21]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x * 1e18) / 1e18, 1164518589284217277370 [1.164e21]) [staticcall]
    │   └─ ← ()
    └─ ← ()

  [7725] POC::testPOC(16826228168456047587 [1.682e19]) 
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x / 1e18) * 1e18, 16000000000000000000 [1.6e19]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x * 1e18) / 1e18, 16826228168456047587 [1.682e19]) [staticcall]
    │   └─ ← ()
    └─ ← ()

  [7725] POC::testPOC(5693222591586418917 [5.693e18]) 
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x / 1e18) * 1e18, 5000000000000000000 [5e18]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log((x * 1e18) / 1e18, 5693222591586418917 [5.693e18]) [staticcall]
    │   └─ ← ()
    └─ ← ()

Recommended Mitigation Steps

Change the update_market function to:

    function update_market(address _market) public {
        require(lendingMarketWhitelist[_market], "Market not whitelisted");
        MarketInfo storage market = marketInfo[_market];
        if (block.number > market.lastRewardBlock) {
            uint256 marketSupply = lendingMarketTotalBalance[_market];
            if (marketSupply > 0) {
                uint256 i = market.lastRewardBlock;
                while (i < block.number) {
                    uint256 epoch = (i / BLOCK_EPOCH) * BLOCK_EPOCH; // Rewards and voting weights are aligned on a weekly basis
                    uint256 nextEpoch = i + BLOCK_EPOCH;
                    uint256 blockDelta = Math.min(nextEpoch, block.number) - i;
                    uint256 cantoReward = (blockDelta *
                        cantoPerBlock[epoch] *
-                       gaugeController.gauge_relative_weight_write(_market, epoch)) / 1e18;
-                   market.accCantoPerShare += uint128((cantoReward * 1e18) / marketSupply);
+                       gaugeController.gauge_relative_weight_write(_market, epoch)) * 1e18;
+                   market.accCantoPerShare += uint128((cantoReward / 1e18) / marketSupply);
                    market.secRewardsPerShare += uint128((blockDelta * 1e18) / marketSupply); // TODO: Scaling
                    i += blockDelta;
                }
            }
            market.lastRewardBlock = uint64(block.number);
        }
    }

Assessed type

Math

Low

[L-01] Hard-coded block rate

If the block generation rate ever changes in the Canto network, it will lead to epochs being shorter or longer than expected, which affects directly to the amount of CANTO rewards and secundary rewards per share:

LendingLedger, line 11

    uint256 public constant BLOCK_EPOCH = 100_000; // 100000 blocks, roughly 1 week

LendingLedger, lines 64 to 71

    function update_market(address _market) public {
        ...

                    uint256 epoch = (i / BLOCK_EPOCH) * BLOCK_EPOCH; // Rewards and voting weights are aligned on a weekly basis
                    uint256 nextEpoch = i + BLOCK_EPOCH;
                    uint256 blockDelta = Math.min(nextEpoch, block.number) - i;
                    uint256 cantoReward = (blockDelta *
                        cantoPerBlock[epoch] *
                        gaugeController.gauge_relative_weight_write(_market, epoch)) / 1e18;
                    market.accCantoPerShare += uint128((cantoReward * 1e18) / marketSupply);
                    market.secRewardsPerShare += uint128((blockDelta * 1e18) / marketSupply); // TODO: Scaling
        
        ...
        
    }

Consider making it a non-constant variable which can be set by governance:

    uint256 public BLOCK_EPOCH = 100_000; // 100000 blocks, roughly 1 week

    function setBlockEpoch(uint256 epoch) external onlyGovernance {
        BLOCK_EPOCH = epoch;
    }

Non-critical

[NC-01] No error string in require

LendingLedger, modifier onlyGovernance

    modifier onlyGovernance() {
        require(msg.sender == governance);
        _;
    }

Change it to something like:

    modifier onlyGovernance() {
        require(msg.sender == governance, "Not authorized");
        _;
    }

[NC-02] Missing documentation in update_market

There are no comments explaining what does update_marked. Consider adding a brief explanation.

[NC-03] Missing events emitted in critical areas

Consider emitting events when doing changes to state variables like governance or when a market is being whitelisted, so that off-chain actors can keep track of the contract state:

[NC-04] Unused imports

Namely, VotingEscrow, IERC20 and SafeERC20:

import {VotingEscrow} from "./VotingEscrow.sol";
import {GaugeController} from "./GaugeController.sol";
import {Math} from "@openzeppelin/contracts/utils/math/Math.sol";
import {IERC20, SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";