code-423n4 / 2022-01-sandclock-findings

0 stars 0 forks source link

[WP-L6] Critical operations should emit events #162

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

Across the contracts, there are certain critical operations that change critical values that affect the users of the protocol.

It's a best practice for these setter functions to emit events to record these changes on-chain for off-chain monitors/tools/interfaces to register the updates and react if necessary.

Instances include:

https://github.com/code-423n4/2022-01-sandclock/blob/a90ad3824955327597be00bb0bd183a9c228a4fb/sandclock/contracts/Vault.sol#L103-L119

function setStrategy(address _strategy)
    external
    override(IVault)
    requiresTrust
{
    require(_strategy != address(0), "Vault: strategy 0x");
    require(
        IStrategy(_strategy).vault() == address(this),
        "Vault: invalid vault"
    );
    require(
        address(strategy) == address(0) || strategy.investedAssets() == 0,
        "Vault: strategy has invested funds"
    );

    strategy = IStrategy(_strategy);
}
0xBRM commented 2 years ago

Not a risk.

dmvt commented 2 years ago

Event handling issues are non-critical

naps62 commented 2 years ago

@gabrielpoca I'm not sure if this solved already?

gabrielpoca commented 2 years ago

I think so, but it would be better if you could have a look to confirm.

naps62 commented 2 years ago

Confirmed. seems to be fixed now