search

code-423n4 / 2021-09-sushitrident-findings

0 stars 0 forks source link

Unconditional setting of boolean/address values is risky #56

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The addToWhitelist and removeFromWhitelist unconditionally set the whitelist values of whitelistedFactories for the requested factory without checking if _factory was already whitelisted or not. Checking before setting will help surface inconsistent views of contract state vs offchain/owner assumptions.

The same is true for setMigrator function where it will help to check that the address being set is different from the one already set.

Proof of Concept

https://github.com/sushiswap/trident/blob/6bd4c053b6213ffc612987eb565aa3813d5f0d13/contracts/deployer/MasterDeployer.sol#L49-L57

https://github.com/sushiswap/trident/blob/6bd4c053b6213ffc612987eb565aa3813d5f0d13/contracts/deployer/MasterDeployer.sol#L65-L68

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add a require to check that the whitelist is false/true before setting it to true/false. Add a require to check that _migrator != migrator.

maxsam4 commented 3 years ago

Acceptable risk. I don't see how this will cause irregularities in offchain workers.