Platform: Code4rena
Start Date: 15/10/2021
Pot Size: $35,000 USDC
Total HM: 4
Participants: 10
Period: 4 days
Judge: GalloDaSballo
Total Solo HM: 2
Id: 38
League: ETH
Rank: 5/10
Findings: 2
Award: $1,315.92
🌟 Selected for report: 4
🚀 Solo Findings: 0
🌟 Selected for report: cmichel
Also found by: pmerkleplant
345.9567 USDC - $345.96
pmerkleplant
If address(0) is set as priviledged in Identity.sol, every address can
execute arbitrary transactions through the execute() function.
There is no check in either Identity's constructor nor in the
setAddrPrivilege() function to prevent a user from setting the address(0) as
priviledged.
The execute function only requires
that privileges[signer] != bytes32(0), i.e. the signer is privileged.
Before the check, the signer is assigned as the return value from
SignatureValidator::recoverAddrImpl().
This function however returns the address(0) i.a. if the SignatureMode is
NoSig (here).
The SignatureMode only depends on the signature
argument which the caller provides in the execute function, and therefore
has total control of.
Missing checks to disallow setting the address(0) as priviledged:
Add checks in Identity.sol's constructor and setAddrPrivilege function
to disallow setting the address(0) as privileged.
#0 - Ivshti
2021-10-19T16:28:36Z
#1 - GalloDaSballo
2021-10-25T00:36:44Z
At the root of it all, the initial vulnerability was the fact that erecover would return address(0) when the signature provided was invalid
This has been mitigated
That said this finding specifies a potential attack under specific conditions
I do agree with the attack given the conditions
The question that is unanswered is whether it is possible for these conditions (privileges[address(0)] == 1) to ever be true
I'm thinking a user could maliciously be tricked into approving address(0)
This is the equivalent of being tricked into giving out an allowance to a malicious contract
In that case we wouldn't say that the approve function from the ERC20 standard is vulnerable or exploitable, we would just say that giving the approval to a malicious contract can be dangerous
#2 - Ivshti
2021-10-25T01:05:22Z
@GalloDaSballo nonetheless, after the mitigation it's not possible for any sig type to produce a signature for address(0) (except Spoof, but that's only enabled when tx.origin itself is 0x00..01 and in specific conditions)
so regardless of whether the user is tricked into this, it wouldn't matter
#3 - GalloDaSballo
2021-10-25T11:10:11Z
Agree that the exploit has been mitigated
For the sake of awarding prizes I still have to evaluate the merits of this finding
I'll mark this as Duplicate of #37 and set the severity to low
🌟 Selected for report: pmerkleplant
61.5157 USDC - $61.52
pmerkleplant
Variable DOMAIN_SEPARATOR in the QuickAccManager is never reset after
initialization in the constructor. Declaring it as immutable saves gas.
#0 - GalloDaSballo
2021-10-24T23:02:07Z
Agree with the finding, the sponsor has mitigated this as part of adding multichain compatibility
🌟 Selected for report: pmerkleplant
61.5157 USDC - $61.52
pmerkleplant
Variable creator in the IdentityFactory is never reset after
initialization in the constructor. Declaring it as immutable saves gas.
#0 - Ivshti
2021-10-19T16:06:45Z
#1 - GalloDaSballo
2021-10-24T23:03:23Z
Agreed
The sponsor has applied the improvment
🌟 Selected for report: pmerkleplant
61.5157 USDC - $61.52
pmerkleplant
Variable CANCEL_PREFIX in the QuickAccManager is never reset after
initialization. Declaring it as a constant saves gas.
#0 - Ivshti
2021-10-19T16:07:15Z
#1 - GalloDaSballo
2021-10-24T23:03:41Z
The sponsor has applied the improvement
🌟 Selected for report: WatchPug
Also found by: pauliax, pmerkleplant
16.6093 USDC - $16.61
pmerkleplant
The variables admin, lendingPool, aaveRefCode and allowedSpenders are
all set in the constructor and never changed again.
Declaring them as immutable saves gas.
#0 - Ivshti
2021-10-19T19:00:09Z
fixed in https://github.com/AmbireTech/adex-protocol-eth/commit/03b6cf72c4c167b320133bc8ab0bd2d0e3ec5f65
except allowedSpenders cannot be immutable cause it's a mapping
#1 - GalloDaSballo
2021-10-24T22:27:23Z
Duplicate of #29
🌟 Selected for report: pmerkleplant
768.7927 USDC - $768.79
pmerkleplant
If a caller has privileges for a QuickAccount consisting of two address(0)'s,
then the caller can execute arbitrary transactions through the
QuicAccManager::send() function.
A caller of the QuickAccManager::send() needs to be privileged for the
QuickAccount the caller provides as argument (line 57).
As an arbitrary value can be set as privileged[caller] in Indentity.sol, so
can a QuickAccount struct consisting of two address(0)'s.
The following calls to SignatureValidator::recoverAddr() (line 69, 70 and 73)
can be made to always return address(0) if the signature has
SignatureMode.NoSig.
As the signature is provided as argument in QuickAccManager::send(), the
caller has total control of it.
The checks in line 69, 70
and 73
now always pass as long as the accounts in the QuickAccount struct are
address(0) too.
Therefore, a caller with permissions for such a QuickAccount can execute and schedule arbitrary transactions without the need for valid signatures.
Add a check in the QuickAccManager::send() function to forbid
QuickAccounts with address(0).
#0 - Ivshti
2021-10-19T16:29:16Z
#1 - GalloDaSballo
2021-10-25T11:15:03Z
This is in line with the root problem of ecrecover returning address(0) on invalid signature
I'm assuming a quickaccount with 0, 0 can exist, but it would not contain any user funds
Downgrading to low severity
Underlying issue has been mitigated by the sponsor