SKALE contest - 0x1f8b's results

Duplicate of #16, sponsor disputed, see #16

Reference https://github.com/code-423n4/2022-02-skale-findings/issues/8, https://github.com/code-423n4/2022-02-skale-findings/issues/9, https://github.com/code-423n4/2022-02-skale-findings/issues/10, https://github.com/code-423n4/2022-02-skale-findings/issues/12, https://github.com/code-423n4/2022-02-skale-findings/issues/19 #42

Only standard functions are applied to out-of-the-box DepositBoxes and TokenManagers. It's up to the SKALE chain owner's discretion to create custom DepositBoxes/TokenManagers that specifically support non-standard functions like Rebasing/Deflationary/Inflationary tokens, and add these custom contracts to the bridge using registerExtraContract() functions in MessageProxy.

Because this is reliant on configuration, I believe the finding to be valid and of medium severity.

End users can verify if the DepositBoxes are properly handling rebasing tokens at the time they wish to bridge

4. Initialization is performed automatically by a script
5. It's just an example and should not be used in production

Used an outdated solidity compiler with known bugs.

Agree because of actual evidence, would recommend the sponsor to update to 0.8.9+ (0.8.11 seems to be fairly used)

It seems that was used an outdated version of openzeppelin, the last version is 4.5.0

I think the finding has validity but I wouldn't stress too much on that, would recommend diffind the codebases for any patches

Depositbox whitelist is enabled by default, so it's more a black list than a whitelist.

Don't think it's a vulnerability

There are a lack of input checks around the contracts:

Finding is valid

The logic applied to emit an event of the change of a variable, does not check that the change is to the same value as the current one, it should be omitted to launch a change event if the defined value is the same, otherwise, the dApps could have wrong logics

Agree but non-critical / informational

Is not possible to change the owner, this is a very bad practice, the private key could be leaked, or the admin could be revoked.

Appreciate the finding, but believe this is a testing contract

Only marginal gas improvements.

Require messages gas savings. I'll detail my reasoning here and then link to this for my thoughts.

Ultimately this is saving a small amount of runtime gas but it's saving a pretty hefty amount of gas for deployment.

If we assume that 32bytes costs 20k gas (SSTORE), then each extra char will save 625 gas.

As a convention in this context, I'll rate each shortened message with 2.5k of gas savings although a more correct estimate would be CHAR * 625

Shorten Messages

21 Shortened Messages * 2500 = 52500

i++ to ++i

Saves 3 gas * 25 = 75

### Use of immutable This would not only save the SSTORE but also save on the SLOAD each time the functions are called, in lack of further details I'll give one SLOAD per variable 5 * 2100 = 10500

Unused return

I don't believe this saves gas

Inline 2 functions

Would save 8 * 2 = 16 gas

Cache the constant value keccak256(xxxxxxxxxxxx) as a constant

Would save 30 gas for each time this is done = 30 * 4 = 120

Use delete instead of set to default value (false or 0)

Doesn't save gas

allowance not needed. The transferFrom already check the expected amount.

Agree that the check is not necessary, would save a STATICCALL (100) + a Hot SLOAD (100) = 200 * 2 = 400

The data variable is assigned, but in the case of isMainChainToken=true, the previous content will be discarded and the calculation could be saved if done inside the else block

Skipping the extra computation would save 100 gas (STATICCALL) as well as the cost of copying from memory again (3 * params=4 = 12) 112

Total Gas Saved: 63648