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: 2/13
Findings: 3
Award: $11,088.93
🌟 Selected for report: 5
🚀 Solo Findings: 1
🌟 Selected for report: WatchPug
2.9984 ETH - $10,723.39
WatchPug
// this is used to have the contract upgradeable function initialize(uint128 minStakedRequired) public initializer {
Based on the context and comments in the code, the DelegatedStaking.sol contract is designed to be deployed as an upgradeable proxy contract.
However, the current implementaion is using an non-upgradeable version of the Ownbale library: @openzeppelin/contracts/access/Ownable.sol instead of the upgradeable version: @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol.
A regular, non-upgradeable Ownbale library will make the deployer the default owner in the constructor. Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts. Therefore, there will be no owner when the contract is deployed as a proxy contract.
As a result, all the onlyOwner functions will be inaccessible.
Use @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol and @openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol instead.
And change the initialize() function to:
function initialize(uint128 minStakedRequired) public initializer { __Ownable_init(); ... }
#0 - GalloDaSballo
2021-10-30T00:19:06Z
Agree with the finding, when using Upgradeable Proxies it's important to use the adequate libraries that will be compatible with initializable contracts
#1 - GalloDaSballo
2021-10-30T00:22:29Z
The sponsor has mitigated the issue
0.0082 ETH - $29.39
WatchPug
For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.
For example:
rewardsLocked -= amount;
totalGlobalShares += globalSharesToAdd; v.globalShares += globalSharesToAdd;
us.amount -= amount;
validatorsN +=1;
us.amount -= amount;
us.amount -= amount;
uint128 commissionLeftOver = amount < v.commissionAvailableToRedeem ? v.commissionAvailableToRedeem - amount : 0; // if there is more, redeem it from regular rewards if (commissionLeftOver == 0){ uint128 validatorSharesRemove = tokensToShares(amount - v.commissionAvailableToRedeem, v.exchangeRate);
#0 - GalloDaSballo
2021-11-01T00:42:32Z
Agree with the finding, the sponsor has applied the improvement in a subsequent PR
WatchPug
For public functions, the input parameters are copied to memory automatically which costs gas. If a function is only called externally, making its visibility as external will save gas because external functions’ parameters are not copied into memory and are instead read from calldata directly.
For example:
#0 - kitti-katy
2021-10-21T19:24:16Z
duplicate of #2
#1 - GalloDaSballo
2021-11-01T17:15:45Z
Duplicate of #2
🌟 Selected for report: WatchPug
Also found by: harleythedog, pants
0.0082 ETH - $29.39
WatchPug
For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).
For example:
if (msg.sender == v._address){ require(amount + v.stakings[msg.sender].staked >= validatorMinStakedRequired, "Amount is less than minimum staked required"); } else { // otherwise need to check for max cap uint128 validatorStaked = v.stakings[v._address].staked;
v._address is read twice.
function addValidator(address validator, address operator, uint128 commissionRate) public onlyOwner { validators[validatorsN]._address = validator; validators[validatorsN].operator = operator; validators[validatorsN].commissionRate = commissionRate; emit ValidatorAdded(validatorsN, validator, operator); validatorsN +=1; }
validatorsN is read 4 times.
function getValidatorsDetails() public view returns (uint128[] memory commissionRates, uint128[] memory delegated) { commissionRates = new uint128[](validatorsN); delegated = new uint128[](validatorsN); for (uint128 i = 0; i < validatorsN; i++){ Validator storage v = validators[i]; commissionRates[i] = v.commissionRate; delegated[i] = v.delegated - v.stakings[v._address].staked; } return (commissionRates, delegated); }
validatorsN is read 3 times.
function getDelegatorDetails(address delegator) public view returns( uint[] memory delegated, uint[] memory rewardsAvailable, uint[] memory commissionRewards) { delegated = new uint[](validatorsN); rewardsAvailable = new uint[](validatorsN); commissionRewards = new uint[](validatorsN); uint256 currentEpoch = block.number < endEpoch? block.number: endEpoch; uint128 newGlobalExchangeRate = uint128((uint256(allocatedTokensPerEpoch) * divider/totalGlobalShares)*(currentEpoch - lastUpdateEpoch)) + globalExchangeRate; for (uint128 i = 0; i < validatorsN; i++){ Validator storage v = validators[i];
validatorsN is read 4 times.
if(msg.sender == v._address){ require(rewards + v.commissionAvailableToRedeem >= amount, "Cannot redeem more than available"); // first redeem rewards from commission uint128 commissionLeftOver = amount < v.commissionAvailableToRedeem ? v.commissionAvailableToRedeem - amount : 0; // if there is more, redeem it from regular rewards if (commissionLeftOver == 0){ uint128 validatorSharesRemove = tokensToShares(amount - v.commissionAvailableToRedeem, v.exchangeRate); s.shares -= validatorSharesRemove; v.totalShares -= validatorSharesRemove; }
v.commissionAvailableToRedeem is read 4 times.
#0 - kitti-katy
2021-10-26T18:43:53Z
sort of duplicate of https://github.com/code-423n4/2021-10-covalent-findings/issues/38
#1 - GalloDaSballo
2021-11-01T16:44:37Z
Marking this as primary as it shows the problem as well as the solution
#2 - GalloDaSballo
2021-11-01T16:46:03Z
Agree that you may want to store storage variables that are going to be read more than once in supporting memory variables
Each read from memory costs just 3 gas while each subsequent read from Storage will cost at least 100 (the first one cost 800 iirc)
I appreciate that the warden showed snippets so simplify applying the improvement
WatchPug
Some storage variables include divider, validatorCoolDown, delegatorCoolDown, maxCapMultiplier, allocatedTokensPerEpoch, globalExchangeRate and CQT are only set once in initialize() and never changed.
Changing them to constant can save gas.
Consider changing to constant and using all caps for the names.
uint256 constant DIVIDER = 10**18; // 18 decimals uint128 constant VALIDATOR_COOL_DOWN = 180*6646; // ~ 6 months uint128 constant DELEGATOR_COOL_DOWN = 28*6646; // ~ 28 days uint128 constant MAX_CAP_MULTIPLIER = 10; uint128 constant ALLOCATED_TOKENS_PER_EPOCH = 1*10**18; // should never be 0 uint128 constant GLOBAL_EXCHANGE_RATE = 10**18; // 1 to 1 IERC20 CQT = IERC20(0xD417144312DbF50465b1C641d016962017Ef6240);
#0 - kitti-katy
2021-10-21T19:39:57Z
DIVIDER should be set to constant and there is a duplicate #27
These are changeable, (either in setters or potentially in future upgrade)
uint128 constant DELEGATOR_COOL_DOWN = 28*6646; // ~ 28 days uint128 constant MAX_CAP_MULTIPLIER = 10; uint128 constant ALLOCATED_TOKENS_PER_EPOCH = 1*10**18; // should never be 0 uint128 constant GLOBAL_EXCHANGE_RATE = 10**18; // 1 to 1
#1 - GalloDaSballo
2021-11-01T17:12:24Z
Duplicate of #27
WatchPug
The variable epochs is only used when endEpoch != 0, move it into the block of if (endEpoch != 0){ ... } can avoid unnecessary code execution and save some gas.
function takeOutRewardTokens(uint128 amount) public onlyOwner { require(amount > 0, "Amount is 0"); uint128 currentEpoch = uint128(block.number); uint128 epochs = amount / allocatedTokensPerEpoch; if (endEpoch != 0){ require(endEpoch - epochs > currentEpoch, "Cannot takeout rewards from past"); endEpoch = endEpoch - epochs; } else{ require(rewardsLocked >= amount, "Amount is greater than available"); rewardsLocked -= amount; } transferFromContract(owner(), amount); emit AllocatedTokensTaken(amount); }
Change to:
function takeOutRewardTokens(uint128 amount) public onlyOwner { require(amount > 0, "Amount is 0"); uint128 currentEpoch = uint128(block.number); if (endEpoch != 0){ uint128 epochs = amount / allocatedTokensPerEpoch; require(endEpoch - epochs > currentEpoch, "Cannot takeout rewards from past"); endEpoch = endEpoch - epochs; } else{ require(rewardsLocked >= amount, "Amount is greater than available"); rewardsLocked -= amount; } transferFromContract(owner(), amount); emit AllocatedTokensTaken(amount); }
#0 - kitti-katy
2021-10-21T19:35:50Z
duplicate of #26
#1 - GalloDaSballo
2021-11-01T17:15:18Z
Duplicate of #26
🌟 Selected for report: WatchPug
0.0304 ETH - $108.84
WatchPug
function disableValidator(uint128 validatorId) public { Validator storage v = validators[validatorId]; require(v.disabledEpoch == 0, "Validator is already disabled"); require(v._address == msg.sender || msg.sender == owner(), "Caller is not the owner or the validator"); require(validatorId < validatorsN, "Invalid validator"); updateGlobalExchangeRate(); updateValidator(v); v.disabledEpoch = uint128(block.number) < endEpoch? uint128(block.number) : endEpoch; totalGlobalShares -= v.globalShares; emit ValidatorDisabled(validatorId); }
The check of validatorId < validatorsN can be done earlier to avoid reading from storage when validatorId is invalid.
Change to:
function disableValidator(uint128 validatorId) public { require(validatorId < validatorsN, "Invalid validator"); Validator storage v = validators[validatorId]; require(v.disabledEpoch == 0, "Validator is already disabled"); require(v._address == msg.sender || msg.sender == owner(), "Caller is not the owner or the validator"); updateGlobalExchangeRate(); updateValidator(v); v.disabledEpoch = uint128(block.number) < endEpoch? uint128(block.number) : endEpoch; totalGlobalShares -= v.globalShares; emit ValidatorDisabled(validatorId); }
#0 - kitti-katy
2021-10-21T19:37:44Z
doesn't even need to check it. There is another solution. #3
#1 - GalloDaSballo
2021-11-01T17:14:00Z
While the solution proposed is not the one the sponsor used, the finding is valid in that you can make the function fail earlier
🌟 Selected for report: WatchPug
0.0304 ETH - $108.84
WatchPug
function takeOutRewardTokens(uint128 amount) public onlyOwner { require(amount > 0, "Amount is 0"); uint128 currentEpoch = uint128(block.number); uint128 epochs = amount / allocatedTokensPerEpoch; if (endEpoch != 0){ require(endEpoch - epochs > currentEpoch, "Cannot takeout rewards from past"); endEpoch = endEpoch - epochs; } else{ require(rewardsLocked >= amount, "Amount is greater than available"); rewardsLocked -= amount; } transferFromContract(owner(), amount); emit AllocatedTokensTaken(amount); }
Since the takeOutRewardTokens() function is onlyOwner, transferFromContract(owner(), amount); can be changed to transferFromContract(msg.sender, amount); to avoid unnecessary internal call and storage read to save some gas.