Canto Invitational
Findings & Analysis Report
2024-02-07
Table of contents
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit outlined in this document, C4 conducted an analysis of the Canto Invitational smart contract system written in Solidity. The audit took place between January 25—January 29 2024.
Wardens
In Code4rena’s Invitational audits, the competition is limited to a small group of wardens; for this audit, 5 wardens contributed reports:
This audit was judged by Alex the Entrprenerd.
Final report assembled by PaperParachute.
Summary
The C4 analysis yielded an aggregated total of 4 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 2 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 5 reports detailing issues with a risk rating of LOW severity or non-critical.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 Canto Invitational repository, and is composed of 1 smart contract written in the Solidity programming language and includes 106 lines of Solidity code.
In addition to the known issues identified by the project team, an Automated Findings report was generated using the 4naly3er bot and all findings therein were classified as out of scope.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.
High Risk Findings (2)
[H-01] update_market() market weight incorrect
Submitted by bin2chen, also found by xuwinnie
In update_market()
We need to get the weight percentage of the corresponding market epoch through gaugeController
.
Then allocate cantoPerBlock[epoch]
according to the percentage
The main logic code is as follows:
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);
}
}
- Calculate
epoch
- Then get the corresponding
weight
of the market throughgaugeController.gauge_relative_weight_write(market,epoch)
The problem is that epoch
is block number
market.lastRewardBlock = uint64(block.number)
uint256 epoch = (i / BLOCKEPOCH) * BLOCKEPOCH
But the second parameter of gaugeController.gauge_relative_weight_write(market,epoch)
is time
gauge_relative_weight_write()
->_gauge_relative_weight()
contract GaugeController {
@> uint256 public constant WEEK = 7 days;
...
function _gauge_relative_weight(address _gauge, uint256 _time) private view returns (uint256) {
@> uint256 t = (_time / WEEK) * WEEK;
uint256 total_weight = points_sum[t].bias;
if (total_weight > 0) {
uint256 gauge_weight = points_weight[_gauge][t].bias;
return (MULTIPLIER * gauge_weight) / total_weight;
} else {
return 0;
}
}
For example, the current canto BLOCK: 7999034
After calculation in gaugeController
, it is: 7999034 / WEEK * WEEK = 1970-04-02
The wrong time cycle, the weight obtained is basically 0, so the reward cannot be obtained.
Impact
Incorrectly using block number
as a time parameter, unable to get weight
, unable to accumulate rewards.
Recommended Mitigation
It is recommended that LendingLedger
refer to GaugeController
, and also use time
to record epoch
.
OpenCoreCH (Canto) confirmed and commented:
True, this was time in the past, will be changed.
Alex the Entreprenerd (Judge) commented:
The finding shows how, due to using the incorrect units (blocks instead of seconds), it is possible to cause the
cantoReward
math to be incorrect.Based on the block the result would either cause a total loss of rewards (example from the warden) or an incorrect amount.
While the impact is limited to rewards, the incorrect formula seems to warrant a High Severity.
[H-02] update_market() nextEpoch calculation incorrect
Submitted by bin2chen, also found by xuwinnie
A very important logic of update_market()
is to update accCantoPerShare
.
When updating, if it crosses the epoch boundary, it needs to use the corresponding epoch’s cantoPerBlock[epoch]
.
For example:
cantoPerBlock[100000] = 100
cantoPerBlock[200000] = 0 (No reward)
lastRewardBlock = 199999
block.number = 200002
At this time, accCantoPerShare
needs to be increased:
= cantoPerBlock[100000] * 1 Delta + cantoPerBlock[200000] *2 Delta
= 100 * (200000 - 199999) + 0 * (200002 - 200000) = 100
The code is as follows:
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);
}
}
The above code, the calculation of nextEpoch
is wrong
uint256 nextEpoch = i + BLOCK_EPOCH;
The correct one should be
uint256 nextEpoch = epoch + BLOCK_EPOCH;
As a result, the increase of accCantoPerShare
becomes:
= cantoPerBlock[100000] * 3 Delta
= 100 * (200002 - 199999) = 300
Impact
The calculation of update_market()
is incorrect when crossing the epoch boundary, causing the calculation of accCantoPerShare
to be incorrect, which may increase the allocated rewards.
Proof of Concept
The following code demonstrates the above example, when added to LendingLedgerTest.t.sol:
function test_ErrorNextEpoch() external {
vm.roll(fromEpoch);
whiteListMarket();
//only set first , second cantoPerBlock = 0
vm.prank(goverance);
ledger.setRewards(fromEpoch, fromEpoch, amountPerBlock);
vm.prank(lendingMarket);
ledger.sync_ledger(lender, 1e18);
//set lastRewardBlock = nextEpoch - 10
vm.roll(fromEpoch + BLOCK_EPOCH - 10);
ledger.update_market(lendingMarket);
//cross-border
vm.roll(fromEpoch + BLOCK_EPOCH + 10);
ledger.update_market(lendingMarket);
//show more
(uint128 accCantoPerShare,,) = ledger.marketInfo(lendingMarket);
console.log("accCantoPerShare:",accCantoPerShare);
console.log("more:",accCantoPerShare - amountPerEpoch);
}
$ forge test -vvv --match-test test_ErrorNextEpoch
Running 1 test for src/test/LendingLedger.t.sol:LendingLedgerTest
[PASS] test_ErrorNextEpoch() (gas: 185201)
Logs:
accCantoPerShare: 1000100000000000000
more: 100000000000000
Recommended Mitigation
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 nextEpoch = epoch + BLOCK_EPOCH;
Alex the Entreprenerd (Judge) commented:
The Warden has shown a mistake in accounting, due to relying on a spot time, it is possible to influence the math in a way that would result in incorrect accounting.
Because of this I agree with High Severity.
Medium Risk Findings (2)
[M-01] secRewardsPerShare Insufficient precision
Submitted by bin2chen, also found by xuwinnie
We also introduced the field secRewardDebt. The idea of this field is to enable any lending platforms that are integrated with Neofinance Coordinator to send their own rewards based on this value (or rather the difference of this value since the last time secondary rewards were sent) and their own emission schedule for the tokens.
The current calculation formula for secRewardsPerShare
is as follows:
market.secRewardsPerShare += uint128((blockDelta * 1e18) / marketSupply);
marketSupply
is cNOTE
, with a precision of 1e18
So as long as the supply is greater than 1
cNote, secRewardsPerShare
is easily rounded down
to 0
Example:
marketSupply = 10e18
blockDelta = 1
secRewardsPerShare=1 * 1e18 / 10e18 = 0
Impact
Due to insufficient precision, secRewardsPerShare
will basically be 0.
Recommended Mitigation
It is recommended to use 1e27 for secRewardsPerShare
:
market.secRewardsPerShare += uint128((blockDelta * 1e27) / marketSupply);
Alex the Entreprenerd (Judge) commented:
The Warden has shown how, due to incorrect precision, it is possible to create a loss due to rounding down.
Because the loss is limited to yield, Medium Severity seems most appropriate.
OpenCoreCH (sponsor) confirmed
[M-02] Loss of precission when calculating the accumulated CANTO per share
Submitted by erebus
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);
}
}
OpenCoreCH (Canto) confirmed and commented:
The given examples do not directly apply in our case, we divide a number with 36 decimals by
1e18
and then multiply by1e18
again (blockDelta
has 0,cantoPerBlock
18,gauge_relative_weight_write
18). Looking at it again, this is pretty inefficient, but I cannot think of a situation where it causes a non-negligible (higher than a few wei) difference in the end. Will double check that.
Ok so I think it could lead to situations where up to 1 CANTO is not distributed, e.g. if
(blockDelta * cantoPerBlock[epoch] * gaugeController.gauge_relative_weight_write(_market, epoch))
is 0.99e18, it will round down to 0. Not a huge loss, but also not ideal (especially because it can be avoided easily), so will be changed.
Alex the Entreprenerd (Judge) commented:
The Warden has shown how, due to rounding a small loss of yield can accrue.
Due to the small scope of the codebase and the specificity of the finding, I’m leaving as Medium as of now.
Low Risk and Non-Critical Issues
For this audit, 5 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by rvierdiiev received the top score from the judge.
The following wardens also submitted reports: erebus, bin2chen, Stormy, and xuwinnie.
[L-01] setRewards
allows to set rewards for epochs in the past
Description
LendingLedger.setRewards
accepts _fromEpoch
and _toEpoch
params. The only check that function does is if epoch are set as weeks. But there is no check if rewards are set for previous epochs.
Recommendation
Check that provided epochs are in future
[L-02] If setRewards
will be called for same epochs, then cantoPerBlock
will be rewritten
Description
LendingLedger.setRewards
accepts _fromEpoch
and _toEpoch
params. The only check that function does is if epoch are set as weeks. It allows to provide epochs that were already set before. In case if this will happen, which is likely not the case as sponsor claimed, then cantoPerBlock
will be rewritten for the epoch.
Recommendation
Increase cantoPerBlock
for the epoch.
cantoPerBlock[i] += _amountPerBlock;
[L-03] Loss of rewards for users in case if market is blacklisted
Description Issue 1
In case if market was whitelisted, earned rewards and then became blacklisted, then it will be not possible for users to claim already earned rewards anymore, because update_market
function revert for non whitelisted markets.
Recommendation Issue 1
Give ability for users to claim already accrued rewards for blacklisted market. Maybe you should remove require
in the update_market
function. As i guess that blacklisted market will have 0 weight in gauge. Then make sure sync_ledger
checks that msg.sender
is whitelisted market.
Description Issue 2
Any market can be blacklisted. This means that rewards will not accrue for it anymore.
When market is blacklisted, then update_market
is not called for it for last time(and already accrued rewards are not available for users as i described in QA-04). And those undistributed earned rewards are not tracked in any way to be distributed to other markets. Thus, those funds will just stuck.
Recommendation Issue 2
You need to allow user to claim all funds(and when blacklisting you should call update_market
for last time) or you need to track unclaimed funds and redistribute them other markets.
[R-01] Contract doesn’t emit any event
Description
LendingLedger
doesn’t emit any event which can help tracking activity for offline services. For example when market is whitelisted/blacklisted, when rewards per block is set for new epochs, when user claimed rewards or when governance has changed.
Recommendation
Add events.
QA-01
setRewards
allows to set rewards for epochs in the past
LQA-02 If
setRewards
will be called for same epochs, thencantoPerBlock
will be rewritten
LQA-03 Contract doesn’t emit any event
RQA-04 Loss of rewards for users in case if market is blacklisted
LQA-05 Some rewards will be lost in case if market will be blacklisted
See QA-043L 1R
C4 Note: QA-04 and QA-05 have been merged into L-03
OpenCoreCH (sponsor) confirmed
Audit Analysis
For this audit, 1 analysis reports were submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by rvierdiiev received the top score from the judge.
Overview
Canto protocol has redesigned LendingLedger contract to allow users to claim their rewards at any time, so they should not wait when epoch will finish. Also secRewardDebt
variable was introduced to allow third parties to distribute rewards to users based on changes of that value during the time.
Key Components
LendingLedger
contract allows governance to set rewards for the epochs, which should be distributed among whitelisted markets according to their weight in gauge controller. LendingLedger
contract tracks total supply of each whitelisted market in order to calculate correct reward per share for the users. Market’s participants can receive rewards according to the amount of shares that they have in the market. Also each user has secRewardDebt
variable that is calculated to allow third parties to rewards users.
Systemic Risks
Correct decisions from governance
: whitelisted markets and rewards per epoch are set by governance. Thus governance is responsible to validate provided values as it possible to break intended work with incorrect input.
Centralization risk
LendingLedger
has only governance role, which should belong to the DAO. No other party is allowed to change contract settings.
Time spent:
4 hours
Disclosures
C4 is an open organization governed by participants in the community.
C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.