Sovryn Governance Smart Contract Audit
Executive Summary
In January 2021, Sovryn engaged Coinspect to perform a source code review of their new governance, staking and fee sharing contracts. The objective of the audit was to evaluate the security of the smart contracts implementing these features.
The code reviewed was found to be clear, well written, and properly documented. No high risk vulnerabilities were discovered during this audit.
Even though the new features give the protocol users more participation, it is worth noting, by design, centralized roles are still able to control governance decisions, at least until governance abdicates their proposal veto right.
The following issues were identified during the assessment:
These findings were addressed by the Sovryn team and each fix was verified by Coinspect to be correct.
Introduction
Sovryn’s goal is to enable lending, borrowing and margin trading in the RSK blockchain.
This audit focused on the smart contracts that implement the following recently developed features:
- Governance
- Staking
- Vesting
- Sovryn Rewards Token
The audit started on January 4th and was conducted on the PR#63 Governance pull request from the Sovryn Git repository. This pull request includes 310 commits and modified 73 files.
This review focused on the following new Solidity source files shown here with their sha256sum hash:
The changes made to other smart contracts in the platform in order to integrate these new files were reviewed as well.
The following design documentation provided by the Sovryn team was consulted during the assessment:
Assessment
The new governance feature, implemented in the GovernorAlpha.sol and Timelock.sol smart contracts, is based on Compound’s governance model implementation. However, the scheme was modified by Sovryn to incorporate weighted voting power based on staking.
The contracts are compiled with Solidity version 0.5.17. A few compile warnings are emitted at build time, most of them related to shadowed variables and the use of ABIEncodeV2, which was experimental for that Solidity release. Several tests were added for the new functionality, all of these 233 new tests included in the repository pass. Coinspect observed the visibility of some public functions could be changed to external in order to optimize gas usage.
The following sections explore each of new features, and provide a brief description and audit notes for each of them.
Governance
The governance model is based on Compound’s system. Executable governance proposals can be made by anybody with voting power above a certain threshold. These proposals are voted, and if approved, queued in the Timelock contract which allows executing them after a period of time.
Votes can be delegated, and are calculated based on the staking voting power taking into account a proposal’s start block and start time.
Sovryn’s implementation allows the governance guardian role to veto any proposal.
A mechanism for the guardian to abdicate his role is provided.
Staking
The staking contract allows users to deposit tokens for a period of time, in exchange for voting power and fees sharing rights. This contract allows users to delegate their stake to another account. Also, it is possible to stake with a certain schedule, this functionality is used by the vesting contract.
The weightedStaking.sol contract is responsible for voting power calculations for any point in time. In order to save gas and prevent attacks, this is implemented via a checkpointing mechanism with a 2 weeks period.
It is worth noting, users are allowed to withdraw their vested tokens before time with a cost proportional to the time remaining until the original stake finish date. The slashed amount is transferred to the fee sharing contract. The following output from one of the tests provided shows how this punishment mechanism works:
Staked amount: 10000
lock date: 2 (weeks), slashed amount: 360 ( 3.6% )
lock date: 20 (weeks), slashed amount: 930 ( 9.3% )
lock date: 40 (weeks), slashed amount: 1500 ( 15% )
lock date: 60 (weeks), slashed amount: 1950 ( 19.5% )
lock date: 80 (weeks), slashed amount: 2340 ( 23.4% )
lock date: 100 (weeks), slashed amount: 2640 ( 26.4% )
lock date: 120 (weeks), slashed amount: 2850 ( 28.5% )
lock date: 140 (weeks), slashed amount: 2970 ( 29.7% )
lock date: 156 (weeks), slashed amount: 3000 ( 30% )
As a consequence, 30% of the staked amount gets slashed when the depositor decides to withdraw 156 weeks before the stake period is due, this is the maximum staking period and its punishment.
The Staking contract owner, governance, has the ability to allow any address to withdraw anytime without being punished through a vesting whitelist which is used in the governanceWithdrawVesting function.
Additionally, there is an irreversible unlock all tokens flag that can be enabled by the contract owner and disables slashing for all stakes.
A staking proxy contract is also included in the repository, which allows the proxy owner to upgrade the staking contract implementation.
Vesting
The Vesting contracts enforce two kinds of vesting schedules: one is intended for investor’s tokens and the other one for the team’s tokens.
Team vesting is implemented in Vesting.sol, the vested tokens get staked in the staking contract with a determined schedule (cliff, duration and period) via the stakeBySchedule function. As time passes, vested tokens get unlocked according to the vesting schedule. The SOV token owner and the Vesting contract owner are the only ones allowed to withdraw tokens. The withdraw function only allows withdrawing tokens that have been unlocked. There is one exception to this rule: the Staking contract is allowed to withdraw all tokens anytime, including locked ones, through the withdrawGovernance function.
Investor’s vesting is implemented in DevelopmentVesting.sol. In this case, vested tokens do not get staked, the vesting contract just locks the tokens. As a consequence, the investors who deposit tokens in this contract do not get voting or fee sharing rights. Tokens can get deposited or vested in this contract: deposited tokens can be withdrawn anytime, vested tokens are locked in for a period of time. In addition to the contract owner, the token owner is allowed to withdraw tokens from this vesting contract. The withdraw functions can only withdraw unlocked vested tokens or those that were deposited.
The vesting contract owner can unlock all tokens anytime by setting the vesting schedule parameters to zero.
Rewards and fee sharing
The RSOV token (Sovryn Reward Token) goal is to allow users to get rewards through the generation of protocol fees. The mint function accepts SOV tokens and mints the same amount of RSOV tokens. When burning RSOV tokens, the user gets 1/14th of the tokens sent back to him and the rest get staked in the user’s behalf with a schedule of 4 weeks cliff and period and 1 year duration.
The FeeSharingProxy contract is intended to be set as the protocol fee collector. Anybody can invoke the withdrawFees function which uses protocol.withdrawFees to obtain available fees from operations on a certain token, these fees are deposited in the corresponding loanPool. Also, the staking contract sends slashed tokens to this contract. When a user calls the withdraw function, the contract transfers the fee sharing rewards in proportion to the user’s weighted stake since the last withdrawal.
Miscellaneous modifications
The following changes were introduced by the same pull request and were reviewed by Coinspect:
- Added new admin and pauser roles, separate from the contract owner, to the LoanToken contracts.
- Added new admin role to State.sol and ProtocolSettings.sol smart contracts.
- Solidity version bump from ^0.4.15 to ^0.5.17 for MultiSigWallet.sol and the required modifications to support the newer version
- New events were added.
- withdrawFees function added to ISovryn protocol interface
Conclusions and Recommendations
No high risk vulnerabilities were found during this assessment. The source code reviewed was found to be correct and only minor suggestions are made in this report in order to improve code quality.
Even though the main goal of the new features is to offer the protocol users a more decentralized governance model and more participation in decision making, it is worth noting the system will not be fully decentralized when deployed and there are mechanisms in place, such as the guardian’s veto right, that allow certain roles to maintain control of governance, such as the veto mechanism. This is intended by design, and the mechanisms to forfeit centralized control are in place for when the moment comes for the Sovryn team to switch to a completely decentralized governance model.
Another important design decision that should be taken into account is the users ability to withdraw their stakes, with a penalization, after proposing and/or voting an executable governance proposal.
The following list sums up the most important recommendations from this audit:
- Review visibility of functions to make sure all functions that can be declared as external are declared that way in order to save gas.
- Clearly document the roles in the system and their rights to bypass locked tokens withdrawal schedules.
- Clearly document the ability of the guardian role to veto any governance proposal. The process for the guardian to monitor proposals and decide which ones should be vetoed should be documented.
Summary of Findings
Remediations
During January 2021 Coinspect verified the findings reported had been properly addressed by the Sovryn team.
The following table lists the findings that were fixed and the corresponding pull requests:
Findings
Description
In the RSK blockchain implementation, ecrecover’s precompiled contract return value in error scenarios is the value 0xdcc703c0E500B653Ca82273B7BFAd8045D85a470 in contrast with Ethereum’s implementation which returns the 0 address for errors.
The castVoteBySig function in the GovernorAlpha smart contract checks the return value is not address 0:
function castVoteBySig(uint proposalId, bool support, uint8 v, bytes32 r, bytes32 s) public {
bytes32 domainSeparator = keccak256(abi.encode(DOMAIN_TYPEHASH, keccak256(bytes(NAME)), getChainId(), address(this)));
bytes32 structHash = keccak256(abi.encode(BALLOT_TYPEHASH, proposalId, support));
bytes32 digest = keccak256(abi.encodePacked(“\x19\x01”, domainSeparator, structHash));
address signatory = ecrecover(digest, v, r, s);
require(signatory != address(0), “GovernorAlpha::castVoteBySig: invalid signature”);
return _castVote(signatory, proposalId, support);
As a consequence, it is possible to cast a vote using an invalid signature. However, in the current implementation, the invalid voter would not have any voting power associated because he would lack the staking required.
The same scenario takes place in the delegateBySig function in Staking.sol:
bytes32 digest = keccak256(abi.encodePacked(“\x19\x01”, domainSeparator, structHash));
address signatory = ecrecover(digest, v, r, s);
require(signatory != address(0), “Staking::delegateBySig: invalid signature”);
Recommendation
Coinspect recommends checking for ecrecover RSK specific error return value in order to prevent future mistakes; for an example check RSKAddrValidator.sol.
Description
The Staking.sol and GovernorAlpha.sol contracts emit warnings at compile time caused by shadowed declarations:
Staking.sol:393:13: Warning: This declaration shadows an existing declaration.
uint96 currentBalance = currentBalance(account, i);
Staking.sol:228:5: The shadowed declaration is here:
function currentBalance(address account, uint lockDate) internal view returns(uint96) {
GovernorAlpha.sol:171:9: Warning: This declaration shadows an existing declaration.
uint96 proposalThreshold = proposalThreshold();
GovernorAlpha.sol:153:5: The shadowed declaration is here:
function proposalThreshold() public view returns (uint96) {
Staking.sol:393:13: Warning: This declaration shadows an existing declaration.
uint96 currentBalance = currentBalance(account, i);
Staking.sol:228:5: The shadowed declaration is here:
function currentBalance(address account, uint lockDate) internal view returns(uint96) {
GovernorAlpha.sol:171:9: Warning: This declaration shadows an existing declaration.
uint96 proposalThreshold = proposalThreshold();
GovernorAlpha.sol:153:5: The shadowed declaration is here:
function proposalThreshold() public view returns (uint96) {
Coinspect auditors reviewed these warnings and concluded they do not represent an immediate risk.
Recommendation
Even though this issue does not represent a security risk right now, Coinspect recommends modifying the variable names to improve code readability and avoid the compile time warnings.
Description
The error strings used in several functions in the Checkpoints smart contract are incorrect, as the error is caused by an overflow in the stake amount and not in the stake date as the string indicates:
function _decreaseUserStake(address account, uint lockedTS, uint96 value) internal{
uint32 nCheckpoints = numUserStakingCheckpoints[account][lockedTS];
uint96 staked = userStakingCheckpoints[account][lockedTS][nCheckpoints — 1].stake;
uint96 newStake = sub96(staked, value, “Staking::_decreaseUserStake: stakedUntil underflow”);
_writeUserCheckpoint(account, lockedTS, nCheckpoints, newStake);
The following error strings in Checkpoints.sol are incorrect:
uint96 newStake = add96(staked, value, “Staking::_increaseUserStake: stakedUntil overflow”);
uint96 newStake = sub96(staked, value, “Staking::_decreaseUserStake: stakedUntil underflow”);
uint96 newStake = add96(staked, value, “Staking::_increaseDelegateeStake: stakedUntil overflow”);
uint96 newStake = sub96(staked, value, “Staking::_decreaseDailyStake: stakedUntil underflow”);
uint96 newStake = add96(staked, value, “Staking::_increaseDailyStake: stakedUntil overflow”);
uint96 newStake = sub96(staked, value, “Staking::_decreaseDailyStake: stakedUntil underflow”);
Recommendations
Modify the revert error strings to indicate a newStake overflow/underflow was the cause.
Disclaimer
The information presented in this document is provided as is and without warranty. Vulnerability assessments are a “point in time” analysis and as such it is possible that something in the environment could have changed since the tests reflected in this report were run. This report should not be considered a perfect representation of the risks threatening the analysed system, networks and applications.
