Platform: Code4rena
Start Date: 22/07/2021
Pot Size: $80,000 USDC
Total HM: 6
Participants: 14
Period: 7 days
Judge: ghoulsol
Total Solo HM: 3
Id: 21
League: ETH
Rank: 2/14
Findings: 5
Award: $17,918.99
🌟 Selected for report: 10
🚀 Solo Findings: 2
gpersoon
If one of the protocols doesn't have enough funds in its protocolBalance, then _payOffDebt will revert when trying to subtract the debt. This also means the function payOffDebtAll will revert. As this function is called from several other functions, including from the Payout function (via _doSherX), it could block a lot of functions, including Payout.
Normally a low balance shouldn't happen because the sherlock DAO will encourage the protocols to fund their account. However if a protocol would be in trouble and couldn't fund its balance, it would block the smart contracts unnecessarily.
Note1: a workaround would be to increase to balance of the protocol by the sherlock DAO via depositProtocolBalance Note2: a protocol (in panic) could make it worse by withdrawing their balance via withdrawProtocolBalance Note3: it will be even worse if an attacker got access to the protocolAgent account and can do withdrawProtocolBalance
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/libraries/LibPool.sol#L84 function payOffDebtAll(IERC20 _token) external { PoolStorage.Base storage ps = PoolStorage.ps(_token); uint256 blocks = block.number.sub(ps.totalPremiumLastPaid);
uint256 totalAccruedDebt; for (uint256 i = 0; i < ps.protocols.length; i++) { totalAccruedDebt = totalAccruedDebt.add(_payOffDebt(ps, ps.protocols[i], blocks)); } // move funds to the sherX etf ps.sherXUnderlying = ps.sherXUnderlying.add(totalAccruedDebt); ps.totalPremiumLastPaid = uint40(block.number);
}
function _payOffDebt( PoolStorage.Base storage ps,bytes32 _protocol,uint256 _blocks) private returns (uint256 debt) { debt = _accruedDebt(ps, _protocol, _blocks); ps.protocolBalance[_protocol] = ps.protocolBalance[_protocol].sub(debt); // might revert if protocolBalance is too low }
Redesign the functions payOffDebtAll and _payOffDebt, where the block period calculation (with totalPremiumLastPaid) is moved to _payOffDebt and calculated per protocol. If a protocol can't pay, the function _payOffDebt could skip this protocol. Then the sherlock DAO should take appropriate actions to fix the problems.
#0 - Evert0x
2021-07-30T14:45:13Z
#119
#1 - ghoul-sol
2021-08-29T23:42:34Z
Duplicate of #119 so high risk
🌟 Selected for report: gpersoon
3740.2597 USDC - $3,740.26
gpersoon
GovDev.sol has a function updateSolution to upgrade parts of the contract via the Diamond construction. Via updateSolution any functionality can be changed and all the funds can be accessed/rugged. Even if this is well intended the project could still be called out resulting in a reputation risk, see for example: https://twitter.com/RugDocIO/status/1411732108029181960
Note: there is a function transferGovDev which can be used to disable the updateSolution
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/GovDev.sol#L25 function updateSolution(IDiamondCut.FacetCut[] memory _diamondCut,address _init,bytes memory _calldata) external override { require(msg.sender == LibDiamond.contractOwner(), 'NOT_DEV'); return LibDiamond.diamondCut(_diamondCut, _init, _calldata); } }
Apply extra safeguards for example to limit the time period where updateSolution can be used.
#0 - Evert0x
2021-07-30T14:44:11Z
Fair point, although we are not anonymous, we still want to mitigate this risk.
I'm thinking something like this
Downside is that it doesn't allow us to fix potential critical issues fast.
🌟 Selected for report: gpersoon
3740.2597 USDC - $3,740.26
gpersoon
When a large payout occurs, it will lower unallocatedSherX. This could mean some parties might not be able to get their Yield.
The first couple of users (for which harvest is called or which transfer tokens) will be able to get their full Yield, until the moment unallocatedSherX is depleted. The next users don't get any yield at all. This doesn't seem fair.
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/SherX.sol#L309 function doYield(ILock token,address from, address to, uint256 amount) private { ... ps.unallocatedSherX = ps.unallocatedSherX.sub(withdrawable_amount);
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/Payout.sol#L108 function payout( address _payout, IERC20[] memory _tokens, uint256[] memory _firstMoneyOut, uint256[] memory _amounts, uint256[] memory _unallocatedSherX, address _exclude ) external override onlyGovPayout { // all pools (including SherX pool) can be deducted fmo and balance // deducting balance will reduce the users underlying value of stake token // for every pool, _unallocatedSherX can be deducted, this will decrease outstanding SherX rewards // for users that did not claim them (e.g materialized them and included in SherX pool) .... // Subtract from unallocated, as the tokens are now allocated to this payout call ps.unallocatedSherX = ps.unallocatedSherX.sub(unallocatedSherX);
If unallocatedSherX is insufficient to provide for all the yields, only give the yields partly (so that each user gets their fair share).
#0 - Evert0x
2021-07-30T16:11:47Z
Not only unallocatedSherX is subtracted but also sWeight, which is used to calculate the reward. I wrote some extra tests and in my experience the remaining SherX (in the unallocatedSherX variable) is splitted in a fair way.
#1 - Evert0x
2021-08-03T10:19:06Z
Together with gpersoon I discussed both issue #49 and #50 and based on both findings we found a med-risk issue. In case payout() is called with _unallocatedSherX > 0 and a user called harvest() before the payout call. It blocks the user from calling harvest() again. + blocks the lock token transfer.
Mitigations step is to stop calling payout() with _unallocatedSherX > 0
🌟 Selected for report: gpersoon
1246.7532 USDC - $1,246.75
gpersoon
In the function tokenUnload, ps.stakeBalance is only deleted if balance >0 e.g it is deleted if ps.stakeBalance > ps.firstMoneyOut So if ps.stakeBalance == ps.firstMoneyOut then ps.stakeBalance will not be deleted. And then a call to tokenRemove will revert, because it checks for ps.stakeBalance to be 0
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/Gov.sol#L271 function tokenUnload( IERC20 _token, IRemove _native, address _remaining ) external override onlyGovMain { ... uint256 balance = ps.stakeBalance.sub(ps.firstMoneyOut); if (balance > 0) { _token.safeTransfer(_remaining, balance); delete ps.stakeBalance; } .. delete ps.firstMoneyOut;
function tokenRemove(IERC20 _token) external override onlyGovMain { ... require(ps.stakeBalance == 0, 'BALANCE_SET');
Check what to do in this edge case and add the appropriate code.
🌟 Selected for report: gpersoon
1246.7532 USDC - $1,246.75
gpersoon
On several locations in the code precautions are taken not to divide by 0, because this will revert the code. However on some locations this isn't done.
Especially in doYield a first check is done for totalAmount >0, however a few lines later there is an other div(totalAmount) which isn't checked.
The proof of concept show another few examples.
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/SherX.sol#L309 function doYield(ILock token,address from,address to,uint256 amount) private { .. uint256 totalAmount = ps.lockToken.totalSupply(); .. if (totalAmount > 0) { ineglible_yield_amount = ps.sWeight.mul(amount).div(totalAmount); } else { ineglible_yield_amount = amount; } if (from != address(0)) { uint256 raw_amount = ps.sWeight.mul(userAmount).div(totalAmount); // totalAmount could be 0, see lines above
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L295 function activateCooldown(uint256 _amount, IERC20 _token) external override returns (uint256) { ... uint256 tokenAmount = fee.mul(LibPool.stakeBalance(ps)).div(ps.lockToken.totalSupply()); // ps.lockToken.totalSupply() might be 0
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L351 function unstake( uint256 _id, address _receiver, IERC20 _token ) external override returns (uint256 amount) { ... amount = withdraw.lock.mul(LibPool.stakeBalance(ps)).div(ps.lockToken.totalSupply()); // // ps.lockToken.totalSupply() might be 0
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/libraries/LibPool.sol#L67 function stake( PoolStorage.Base storage ps,uint256 _amount, address _receiver ) external returns (uint256 lock) { ... lock = _amount.mul(totalLock).div(stakeBalance(ps)); // stakeBalance(ps) might be 0
Make sure division by 0 won't occur by checking the variables beforehand and handling this edge case.
#0 - Evert0x
2021-07-29T18:42:46Z
if (from != address(0)) { uint256 raw_amount = ps.sWeight.mul(userAmount).div(totalAmount); // totalAmount could be 0, see lines above
If totalAmount == 0, from is always address(0). As no one holds this lockToken and it's being minted
#1 - Evert0x
2021-07-29T18:44:05Z
function activateCooldown(uint256 _amount, IERC20 _token) external override returns (uint256) { ... uint256 tokenAmount = fee.mul(LibPool.stakeBalance(ps)).div(ps.lockToken.totalSupply()); // ps.lockToken.totalSupply() might be 0
Can not be 0, as there is lockToken being transferred require(_amount > 0, 'AMOUNT');, so the value is at least 1.
#2 - Evert0x
2021-07-29T18:45:09Z
function unstake( uint256 _id, address _receiver, IERC20 _token ) external override returns (uint256 amount) { ... amount = withdraw.lock.mul(LibPool.stakeBalance(ps)).div(ps.lockToken.totalSupply()); // // ps.lockToken.totalSupply() might be 0
Can not be 0, as there is lockToken in the contract, waiting to be transferred back to the user
#3 - Evert0x
2021-07-29T18:48:56Z
function stake( PoolStorage.Base storage ps,uint256 _amount, address _receiver ) external returns (uint256 lock) { ... lock = _amount.mul(totalLock).div(stakeBalance(ps)); // stakeBalance(ps) might be 0
Can not be 0 (most of the times), as there are already lockTokens in circulation, which means someone has deposited BUT the balance could be fully depleted because of a payout() call which could make it 0.
Thanks!
🌟 Selected for report: gpersoon
1246.7532 USDC - $1,246.75
gpersoon
The functions getInitialUnstakeEntry contains a for loop that can be unbounded. This would mean it could run out of gas and the function would revert. The array unstakeEntries can be made arbitrarily large by repeatedly calling activateCooldown with a small amount of tokens.
The impact is very low because the array unstakeEntries is separated per user and links to mgs.sender, so you can only shoot yourself in your foot
Additionally the function getInitialUnstakeEntry isn't used in the smart contracts
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/PoolBase.sol#L123
function getInitialUnstakeEntry(address _staker, IERC20 _token) external view override returns (uint256) {
...
for (uint256 i = 0; i < ps.unstakeEntries[_staker].length; i++) {
if (ps.unstakeEntries[_staker][i].blockInitiated == 0) {
function activateCooldown(uint256 _amount, IERC20 _token) external override returns (uint256) { require(_amount > 0, 'AMOUNT'); ... ps.unstakeEntries[msg.sender].push(PoolStorage.UnstakeEntry(uint40(block.number), _amount.sub(fee)) );
Probably accept the situation and add a comment in the function getInitialUnstakeEntry
🌟 Selected for report: gpersoon
1246.7532 USDC - $1,246.75
gpersoon
The function _transfer in SherXERC20.sol allow transfer to address 0. This is usually considered the same as burning the tokens and the Emit is indistinguishable from an Emit of a burn.
However the burn function in LibSherXERC20.sol has extra functionality, which _transfer doesn't have. sx20.totalSupply = sx20.totalSupply.sub(_amount);
So it is safer to prevent _transfer to address 0 (which is also done in the openzeppelin erc20 contract) See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L226
Note: minting from address 0 will not work because that is blocked by the safemath sub in: sx20.balances[_from] = sx20.balances[_from].sub(_amount);
https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/SherXERC20.sol#L118 function _transfer(address _from, address _to, uint256 _amount) internal { SherXERC20Storage.Base storage sx20 = SherXERC20Storage.sx20(); sx20.balances[_from] = sx20.balances[_from].sub(_amount); sx20.balances[_to] = sx20.balances[_to].add(_amount); emit Transfer(_from, _to, _amount); }
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/libraries/LibSherXERC20.sol#L29 function burn(address _from, uint256 _amount) internal { SherXERC20Storage.Base storage sx20 = SherXERC20Storage.sx20(); sx20.balances[_from] = sx20.balances[_from].sub(_amount); sx20.totalSupply = sx20.totalSupply.sub(_amount); emit Transfer(_from, address(0), _amount); }
add something like to following to _transfer of SherXERC20.sol: require(_to!= address(0), "Transfer to the zero address");
Or update sx20.totalSupply if burning a desired operation.
gpersoon
Some of the Base struct entries that store a block.number use the type uint40, for example sherXLastAccrued of PoolStorage.sol. Note: the use of uint40 in GovStorage.sol and PoolStorage doesn't save storage because they are mixed in with uint256.
Also the comparable Base struct entry of SherXStorage.sol uses uint256:
It would be more consistent to use the same type everywhere. And to save storage the structs need to be optimized.
// https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/storage/SherXStorage.sol#L14 struct Base { .. uint256 totalUsdLastSettled; .. uint256 internalTotalSupplySettled; }
.\GovStorage.sol: uint40 unstakeCooldown; .\GovStorage.sol: uint40 unstakeWindow; .\GovStorage.sol: uint40 watsonsSherxLastAccrued;
https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/storage/PoolStorage.sol#L16
struct Base {
...
uint256 unallocatedSherX;
uint16 sherXWeight;
uint40 sherXLastAccrued;
mapping(address => uint256) sWithdrawn;
...
uint256 totalPremiumPerBlock;
uint40 totalPremiumLastPaid;
uint256 sherXUnderlying;
...
}
Use the same type to store block.number values everywhere If you use uint40, then optimize the Base struct so that types smaller than uint256 are next to each other and fit in one slot.
#0 - Evert0x
2021-07-29T19:09:27Z
#85
🌟 Selected for report: gpersoon
229.9835 USDC - $229.98
gpersoon
The functions getTotalUsdPool and viewAccrueUSDPool have the same implementation. It saves some gas on the deployment to integrate these functions. Also the maintenance will be a bit easier.
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/SherX.sol#L42 function getTotalUsdPool() external view override returns (uint256) { SherXStorage.Base storage sx = SherXStorage.sx(); return sx.totalUsdPool.add(block.number.sub(sx.totalUsdLastSettled).mul(sx.totalUsdPerBlock)); }
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/libraries/LibSherX.sol#L18 function viewAccrueUSDPool() public view returns (uint256 totalUsdPool) { SherXStorage.Base storage sx = SherXStorage.sx(); totalUsdPool = sx.totalUsdPool.add(block.number.sub(sx.totalUsdLastSettled).mul(sx.totalUsdPerBlock)); }
Integrate the functions getTotalUsdPool and viewAccrueUSDPool (e.g. keep one and remove the other and update the references)
🌟 Selected for report: gpersoon
229.9835 USDC - $229.98
gpersoon
A small gas optimization is possible by replacing x > 0 with x != 0 provide x is an unsigned integer. As the entire code only uses unsigned integers it can be done on all these locations. The proof of concept shows the locations where the "x > 0" construction is used.
.\facets\Gov.sol: require(_tokens.length > 0, 'ZERO'); .\facets\Gov.sol: if (totalToken > 0) { .\facets\Gov.sol: if (totalFee > 0) { .\facets\Gov.sol: if (balance > 0) { .\facets\Manager.sol: if (ps.sherXUnderlying > 0) { .\facets\Manager.sol: if (usdPerBlock > 0 && _currentTotalSupply == 0) { .\facets\Manager.sol: } else if (usdPool > 0) { .\facets\Payout.sol: if (unallocatedSherX > 0) { .\facets\Payout.sol: if (firstMoneyOut > 0) { .\facets\Payout.sol: if (totalUnallocatedSherX > 0) { .\facets\PoolBase.sol: require(_amount > 0, 'AMOUNT'); .\facets\PoolBase.sol: require(_amount > 0, 'AMOUNT'); .\facets\PoolBase.sol: require(_amount > 0, 'AMOUNT'); .\facets\PoolBase.sol: if (fee > 0) { .\facets\PoolBase.sol: if (_forceDebt && accrued > 0) { .\facets\PoolBase.sol: if (ps.protocolBalance[_protocol] > 0) { .\facets\PoolBase.sol: if (ps.protocolPremium[_protocol] > 0) { .\facets\PoolOpen.sol: require(_amount > 0, 'AMOUNT'); .\facets\PoolStrategy.sol: require(_amount > 0, 'AMOUNT'); .\facets\PoolStrategy.sol: require(_amount > 0, 'AMOUNT'); .\facets\SherX.sol: if (stakeBalance > 0) { .\facets\SherX.sol: require(_amount > 0, 'AMOUNT'); .\facets\SherX.sol: if (totalAmount > 0) { .\facets\SherX.sol: if (withdrawable_amount > 0) { .\libraries\LibSherX.sol: if (total > 0) { .\libraries\LibSherX.sol: if (sherX > 0) { .\libraries\LibSherX.sol: if (sherX > 0) { .\strategies\AaveV2.sol: require(amount > 0, 'ZERO_AMOUNT');
grep
replace x > 0 with x != 0
336.6234 USDC - $336.62
gpersoon
The function payout contains an expression with 3 sequential divs. This is generally not recommended because it could lead to rounding errors / loss of precision. Also a div is usually more expensive than a mul. Also an intermediate division by 0 (if SherXERC20Storage.sx20().totalSupply) == 0) could occur.
//https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/Payout.sol#L108 function payout( .. uint256 deduction = excludeUsd.div(curTotalUsdPool.div(SherXERC20Storage.sx20().totalSupply)).div(10e17);
Verify the formula and replace with something like: uint256 deduction = excludeUsd.mul(SherXERC20Storage.sx20().totalSupply).div( curTotalUsdPool.mul(10e17) )
🌟 Selected for report: gpersoon
1246.7532 USDC - $1,246.75
gpersoon
The function setUnstakeWindow and setCooldown don't check that the input parameter isn't 0. So the values could accidentally be set to 0 (although unlikely). However you wouldn't want the to be 0 because that would allow attacks with flashloans (stake and unstake in the same transaction)
https://github.com/code-423n4/2021-07-sherlock/blob/main/contracts/facets/Gov.sol#L124 function setUnstakeWindow(uint40 _unstakeWindow) external override onlyGovMain { require(_unstakeWindow < 25000000, 'MAX'); // ~ approximate 10 years of blocks GovStorage.gs().unstakeWindow = _unstakeWindow; }
function setCooldown(uint40 _period) external override onlyGovMain { require(_period < 25000000, 'MAX'); // ~ approximate 10 years of blocks GovStorage.gs().unstakeCooldown = _period; }
Check the input parameter of setUnstakeWindow and setCooldown isn't 0