Platform: Code4rena
Start Date: 19/10/2021
Pot Size: $30,000 ETH
Total HM: 5
Participants: 13
Period: 3 days
Judge: GalloDaSballo
Total Solo HM: 4
Id: 43
League: ETH
Rank: 4/13
Findings: 4
Award: $3,833.87
🌟 Selected for report: 3
🚀 Solo Findings: 1
xYrYuYx
Owner will deposit any amount of reward if amount is greater than allocatedTokensPerEpoch. This means that it is possible that owner can sent amount which is not multiplier of allocatedTokensPerEpoch. For example, when allocatedTokensPerEpoch is 1 CQT, owner can deposit 5.5 CQT. In this case 5 CQT can be redeemed during 5 epoch, but 0.5 CQT will be forever locked in the contract.
https://github.com/xYrYuYx/C4-2021-10-covalent/blob/main/test/c4-tests/C4_issues.js#L9 This is my test script to proof that.
The reason is that solidity division is for only uint, and if there is decimals, it will round them off. https://github.com/code-423n4/2021-10-covalent/blob/main/contracts/DelegatedStaking.sol#L93
Hardhat test
Transfer exact amount from owner.
can change Line 96 to transferToContract(msg.sender, newEpoch * allocatedTokensPerEpoch);
Here newEpoch is amount / allocatedTokensPerEpoch;
#0 - kitti-katy
2021-10-21T18:47:38Z
duplicate of #10
#1 - GalloDaSballo
2021-11-08T01:05:50Z
Duplicate of #10
🌟 Selected for report: xYrYuYx
0.8995 ETH - $3,217.02
xYrYuYx
UpdateGlobalExchangeRate has incorrect implementation when totalGlobalShares is zero.
If any user didn't start stake, totalGlobalShares is 0, and every stake it will increase.
but there is possibility that totalGlobalShares can be 0 amount later by unstake or disable validator.
https://github.com/xYrYuYx/C4-2021-10-covalent/blob/main/test/c4-tests/C4_issues.js#L76 This is my test case to proof this issue.
In my test case, I disabled validator to make totalGlobalShares to zero.
And in this case, some reward amount will be forever locked in the contract.
After disable validator, I mined 10 blocks, and 4 more blocks mined due to other function calls,
So total 14 CQT is forever locked in the contract.
Hardhat test
Please think again when totalGlobalShares is zero.
#0 - kitti-katy
2021-10-21T18:58:47Z
That is right, and I think the best solution would be to add a validator instance who is the owner and stake some low amount of tokens in it. This way we can make sure there is no such situation when totalGlobalShares becomes 0 and if everyone unstaked, the owner could take out reward tokens and then unstake / redeem rewards.
Not sure. That could even be marked as "high risk". if the situation happens and not handled right away (taking out reward tokens), then there could be more significant financial loss.
#1 - kitti-katy
2021-10-23T00:41:37Z
marked resolved as it will be manually handled
#2 - GalloDaSballo
2021-11-08T01:15:39Z
The issue found by the warden is straightforward:
Through mix of unstaking and the use of disableValidator the warden was able to lock funds, making them irredemeable
It seems to me that this is caused by the fact that unstake as well as disableValidator will reduce the shares: https://github.com/code-423n4/2021-10-covalent/blob/a8368e7982d336a4b464a53cfe221b2395da801f/contracts/DelegatedStaking.sol#L348`
I would recommend separating the shares accounting from the activation of validator, simply removing the subtraction of global shares in disableValidator would allow them to claim those shares.
The function disableValidator can be called by either the validator or the owner, while onlyOwner can add a new validator
The owner has the ability to perform this type of griefing, as well as a group of validators if they so chose
Due to the specifics of the grief I will rate it of Medium Severity, as per the docs:
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
In this case we have a way to leak value (lock funds) with specific condition (malicious owner or multiple griefing validators)
0.003 ETH - $10.71
xYrYuYx
public functions use higher gas than external function
https://ethereum.stackexchange.com/questions/19380/external-vs-public-best-practices
All of the public functions in the contract are not called internally, so access can be changed to external to reduce gas.
#0 - kitti-katy
2021-10-21T19:54:44Z
Seems like it only improves gas when the arguments are arrays. After trying to switch from public to external, it did not decrease the gas used.
#1 - GalloDaSballo
2021-11-01T00:48:09Z
As per the SO thread, the reason why external saves gas is because with complex (pointer) data structures, this avoids the copy process, which can be very expensive for complex data types
Additionally, changing to external is a best practice as it provides a guarantee of how the function is supposed to be used
0.0055 ETH - $19.84
xYrYuYx
This is too complicated steps to transfer ERC20 token which could use more gas. You don't need to check balance before transfer. If there is no enough balance, it SafeERC20 will revert. Also you don't need to check balance after transfer, because CQT does not have transaction fee.
Since there is no transaction fee in CQT token, you can use OZ SafeERC20 library to send or receive.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L20 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L28
#0 - GalloDaSballo
2021-11-01T16:33:51Z
The sponsor has applied the improvement