search

code-423n4 / 2021-06-gro-findings

0 stars 1 forks source link

The use of tx.origin for smart contract safe list is risky and not generic #53

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

The addSafeAddress() takes an address and adds it to a “safe list". This is used in eoaOnly() to give exemption to safe addresses that are trusted smart contracts, when all other smart contacts are prevented from protocol interaction. The stated purpose is to allow only such partner/trusted smart contract integrations (project rep mentioned Argent wallet as the only one for now but that may change) an exemption from potential flash loan threats.

The eoaOnly() check used during deposits and withdrawals checks if preventSmartContracts is enabled and if so, makes sure the transaction is coming from an integration/partner smart contract. But instead of using msg.sender in the check it uses tx.origin. This is suspect because tx.origin gives the originating EOA address of the transaction and not a smart contract’s address. (This may get even more complicated with the proposed EIP-3074.)

Discussion with the project team indicated that this is indeed not the norm but is apparently the case for their only current (none others planned) integration with Argent wallet where the originating account is Argent’s relayer tx.origin i.e. flow: Argent relayer (tx.origin) => Argent user wallet (msg.sender) => gro protocol while the typically expected flow is: user EOA (tx.origin) => proxy (msg.sender) => gro protocol.

While this has reportedly been verified and tested, it does seem strange and perhaps warrants a re-evaluation because the exemption for this/other trusted integration/partner smart contracts will not work otherwise.

Scenario: Partner contract is added to the safe address for exemption but the integration fails because of the use of tx.origin instead of msg.sender.

Proof of Concept

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L266-L272

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L176-L178

https://github.com/code-423n4/2021-06-gro/blob/091660467fc8d13741f8aafcec80f1e8cf129a33/contracts/Controller.sol#L171-L174

Tools Used

Manual Analysis

Recommended Mitigation Steps

Re-evaluate the use of tx.origin instead of msg.sender.

kitty-the-kat commented 3 years ago

Made for specific partner, so cannot be generic

While this has reportedly been verified and tested, it does seem strange and perhaps warrants a re-evaluation because the exemption for this/other trusted integration/partner smart contracts will not work otherwise.

This SC protection is only temporary as we are aware of EIP-3074, and consideration of how we need to change this/or if we need this at all are ongoing.