search

hats-finance / VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f

LP token lending protocol
MIT License
2 stars 1 forks source link

Address.isContract() is not a reliable way of checking if the input is an EOA #40

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: -- Submission hash (on-chain): 0x5224612aa1dbc3efd9856dd3081df2c4289d2c038ab2958c552958a3c1549563 Severity: high severity

Description:

Impact

Detailed description of the impact of this finding.

The expectation of the noContract is controlled using ! access control with Address.isContract, then because isContract() gets the size of the code length of the account in question by relying on extcodesize/address.code.length, this means that the restriction can be bypassed when deploying a smart contract through the smart contract's constructor call.

https://github.com/hats-finance/VMEX-0x050183b53cf62bcd6c2a932632f8156953fd146f/blob/fb396a3fa412e643de7d8a1fd8a0268ab3a2f993/packages/contracts/contracts/protocol/oracles/VMEXOracle.sol#L242

Recommendation

Modify the code to require(msg.sender == tx.origin);

ksyao2002 commented 1 year ago

Thanks for the information. This is not a vulnerability, as c._curvePool can only be set by VMEX global admins. This check is more of a sanity check on the behalf of the VMEX team, and no funds are at risk. The restriction bypassing is impossible for contracts already deployed.

Nabeel-javaid commented 1 year ago

Thanks for the information. This is not a vulnerability, as c._curvePool can only be set by VMEX global admins. This check is more of a sanity check on the behalf of the VMEX team, and no funds are at risk. The restriction bypassing is impossible for contracts already deployed.

it is impossible for the contracts that is alr deployed, but not for the one that are in the process of deployment

also I do think that it should be atleast L or Med