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: 1/14
Findings: 5
Award: $19,550.14
🌟 Selected for report: 16
🚀 Solo Findings: 1
3366.2338 USDC - $3,366.23
cmichel
The SherXERC20.payOffDebtAll function iterates over all protocols of the token.
If a single project does not have enough funds to cover the premium payments, the transactions come to a halt, see _payOffDebt:
debt = _accruedDebt(ps, _protocol, _blocks); // this can revert tx ps.protocolBalance[_protocol] = ps.protocolBalance[_protocol].sub(debt);
Many core functions require paying off debt first and can therefore revert when a single protocol cannot pay the token premium:
setTokenPricesetProtocolPremiumwithdrawProtocolBalanceredeemThis scenario that a protocol is unable to pay a premium does not seem unlikely especially as there can be many protocols and each protocol can pay premiums in potentially many tokens and have to continuously re-deposit to their account to increase the balance. It is also rather involved to remove the protocol's coverage and remove the premium payments for the token. It requires governance interaction and potentially paying for the accumulated debt themselves.
#0 - Evert0x
2021-07-29T13:26:47Z
This was a design tradeoff. As governance we can see it coming as the balance is slowly draining. But the fact the protocols are able to withdraw the full amount at any time could surprise the governance. (and make the reverts in the functions above happening)
We are thinking to add a rule in the withdrawProtocolBalance to only allow withdrawals with at least 2 days of remaining balance. Allowing enough time for governance calls to remove the protocol.
cmichel
There are ERC20 tokens that may make certain customizations to their ERC20 contracts.
Some tokens charge a certain fee for every transfer() or transferFrom().
Others types are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).
The PoolBase.depositProtocolBalance() function will introduce unexpected balance inconsistencies when comparing internal asset records with external ERC20 token contracts.
The protocol's protocolBalance will include the paid fees which have not been received.
At some point, trying to pay out premiums will revert as the true underlying tokens are less than what's stored in protocolBalance.
One possible mitigation is to measure the asset change right before and after the asset-transferring routines
#0 - Evert0x
2021-07-30T14:56:49Z
#12
🌟 Selected for report: cmichel
3740.2597 USDC - $3,740.26
cmichel
The _doSherX function does not attempt to pay off the accrued premiums ("pay off debt") for most tokens, only for the ones that would otherwise revert the tx:
// Expensive operation, only execute to prevent tx reverts if (amounts[i] > ps.sherXUnderlying) { LibPool.payOffDebtAll(tokens[i]); }
The amounts = LibSherX.calcUnderlying(totalSherX) array is an optimistic view assuming all outstanding, accrued premiums would indeed be paid until now.
However, it could be that a protocol does not have enough balance to pay out these premiums and updating the state using LibPool.payOffDebtAll(tokens[i]); would fail for a token.
An inflated amount is then paid out to the user based on the optimistic calcUnderlying call.
#0 - Evert0x
2021-07-29T13:33:43Z
Fair point, the protocol is optimistic the protocols can payoff their debt.
561.039 USDC - $561.04
cmichel
Usually, the functions to increase the allowance are called increaseAllowance and decreaseAllowance but in SherXERC20 they are called increaseApproval and decreaseApproval
Rename these functions to the more common names.
#0 - Evert0x
2021-07-29T12:46:44Z
#1 - Evert0x
2021-07-29T12:47:22Z
0 non-critical
#2 - ghoul-sol
2021-08-30T00:47:57Z
I agree with warden, low risk looks reasonable here.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The Gov.protocolRemove function iterates over all elements of the tokensSherX array.
The transactions could fail if the arrays get too big and the transaction would consume more gas than the block limit. This will then result in a denial of service for the desired functionality and break core functionality.
The severity is low as only governance can whitelist these tokens but not the protocols themselves.
Keep the array size small.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The SherX.getTotalSherXUnminted function iterates over all elements of the tokensStaker array.
The transactions could fail if the arrays get too big and the transaction would consume more gas than the block limit. This will then result in a denial of service for the desired functionality and break core functionality.
The severity is low as only governance can whitelist these tokens but not the protocols themselves.
Keep the array size small.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The LibPool.payOffDebtAll function iterates over all elements of the ps.protocols array.
The transactions could fail if the arrays get too big and the transaction would consume more gas than the block limit. This will then result in a denial of service for the desired functionality and break core functionality.
The severity is low as only governance can whitelist protocols per token but not the protocols themselves.
Keep the array size small.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The Gov.tokenInit skips the underlying token check if the _token is SHERX:
if (address(_token) != address(this)) { require(_lock.underlying() == _token, 'UNDERLYING'); }
This check should still be performed even for _token == address(this) // SHERX, otherwise, the lock can have a different underlying and potentially pay out wrong tokens.
Verify the underlying of all locks.
#0 - Evert0x
2021-07-29T13:19:56Z
Good catch!
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The _doSherX function does not return the correct precision of sherUsd and it is not the "Total amount of USD of the underlying tokens that are being transferred" that the documentation mentions.
sherUsd = amounts[i].mul(sx.tokenUSD[tokens[i]]);
Instead, the amount is inflated by 1e18, it should divide the amount by 1e18 to get a USD value with 18 decimal precision.
The severity is low as the calling site in payout makes up for it by dividing by 1e18 in the deduction computation.
We still recommend returning the correct amount in _doSherX already to match the documentation and avoid any future errors when using its unintuitive return value.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The PoolBase.unstakeWindowExpiry function allows unstaking tokens of other users.
While the tokens are sent to the correct address, this can lead to issues with smart contracts that might rely on claiming the tokens themselves.
For example, suppose the _to address corresponds to a smart contract that has a function of the following form:
function withdrawAndDoSomething() { uint256 amount = token.balanceOf(address(this)); contract.unstakeWindowExpiry(address(this), id, token); amount = amount - token.balanceOf(address(this)); token.transfer(externalWallet, amount) }
If the contract has no other functions to transfer out funds, they may be locked forever in this contract.
🌟 Selected for report: cmichel
1246.7532 USDC - $1,246.75
cmichel
The setWeights function only stores the uint16 part of _weights[i] in storage (ps.sherXWeight = uint16(_weights[i])).
However, to calculate weightAdd/weightSub the full value (not truncated to 16 bits) is used.
This can lead to discrepancies as the actually added part is different from the one tracked in the weightAdd variable.
#0 - Evert0x
2021-07-29T12:59:38Z
Your recommendation is to do .add(uint16(_weights[i])) for both weightAdd and weightSub?
561.039 USDC - $561.04
cmichel
The SherXERC20.initializeSherXERC20 function has initialize in its name which indicates that it should only be called once to initialize the storage. But it can be repeatedly called to overwrite and update the ERC20 name and symbol.
Consider an initializer modifier or reverting if name or symbol is already set.
561.039 USDC - $561.04
cmichel
The SherXERC20.transfer/transferFrom actions allow transferring tokens to the zero address.
This is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.
#0 - Evert0x
2021-07-29T12:44:12Z
Does it make more sense to include an extra burn() function? As removing the possibility to send to zero address removes the ability to burn.
🌟 Selected for report: cmichel
229.9835 USDC - $229.98
cmichel
The SherX.setWeights accrues SherX for all tokens, but it would suffice to do this for the tokens in _tokens parameter.
#0 - Evert0x
2021-07-29T14:09:07Z
Good point, not resolving for complexity / gas optimization tradeoff
🌟 Selected for report: cmichel
229.9835 USDC - $229.98
cmichel
The Payout.payout function performs a token.transfer call twice for some tokens, once when paying out the token itself and once as part of the SherX index in _doSherX. The _doSherX could just return the underlying amounts, then aggregate and do a single transfer per token.
#0 - Evert0x
2021-07-29T13:42:55Z
True, this was also stated as a comment in the code.
Not resolving as it increases complexity (the function is already too complex IMO). + It's not likely payout will be a function that is used a lot. Paying a little extra gas is fine for this function.
🌟 Selected for report: cmichel
229.9835 USDC - $229.98
cmichel
The SherXERC20.increaseApproval function reads the allowance from memory twice.
It should be read once and then cached and for the event you the cache + _amount value.
103.4926 USDC - $103.49
cmichel
The SherXERC20.decreaseApproval function reads the allowance from memory twice.
It should be read once the newValue should be calculated and stored in a variable. The newValue should then be emitted in the event instead of reading from storage again.
#0 - Evert0x
2021-07-29T17:24:17Z
#81
🌟 Selected for report: cmichel
229.9835 USDC - $229.98
cmichel
The SherXERC20.transferFrom function reads the allowance from memory twice.
It should be read once, cached, and then use that value for the if(cachedAllowance ...) and for the newApproval = ... expressions.
#0 - Evert0x
2021-08-09T12:09:54Z
Shoud be G (gas optimisation)
#1 - ghoul-sol
2021-08-30T01:15:09Z
agree with sponsor