code-423n4 / 2021-10-ambire-findings

0 stars 0 forks source link

Only prepare tx when the fee is present #65

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pauliax

Vulnerability details

Impact

You can only prepare the second tx in function sendTransfer if the t.fee > 0. This will avoid useless external call when there is no fee or the user himself calls this function. txns[1].to = t.token; txns[1].data = abi.encodeWithSelector(IERC20.transfer.selector, msg.sender, t.fee);

Recommended Mitigation Steps

if (t.fee > 0) {...}

Ivshti commented 2 years ago

Fixing this would make the fee path more expensive, since we will have to use a push

GalloDaSballo commented 2 years ago

I'm guessing that adding an if for t.fee being > 0 would allow to skip the extra code / added path in that case In the case in which there is a fee, it would add the extra cost of the if

I think the finding is valid, however its fine to not change

Ivshti commented 2 years ago

@GalloDaSballo most cases would have a fee so we'd spend more gas on reallocating the array

GalloDaSballo commented 2 years ago

@Ivshti Not sure what you mean with reallocating

Wouldn't

if(t.fee > 0){
        Identity.Transaction[] memory txns = new Identity.Transaction[](2);
        txns[0].to = t.token;
        txns[0].data = abi.encodeWithSelector(IERC20.transfer.selector, t.to, t.amount);
        txns[1].to = t.token;
        txns[1].data = abi.encodeWithSelector(IERC20.transfer.selector, msg.sender, t.fee);
        identity.executeBySender(txns);
} else {
        Identity.Transaction[] memory txns = new Identity.Transaction[](1);
        txns[0].to = t.token;
        txns[0].data = abi.encodeWithSelector(IERC20.transfer.selector, t.to, t.amount);
        identity.executeBySender(txns);
}

allow you to save gas when there's no fee?

That said, the savings here are just the 3 gas per MSTORE The real cost is in avoiding a CALL when you are transferring 0 tokens

Ivshti commented 2 years ago

hmm good point, I was thinking of it wrong (dynamic sized array and .push)

Still, we don't see a realistic scenario in which this is called without fee so we'll choose not to implement this

GalloDaSballo commented 2 years ago

Makes sense, agree with the sponsor in not having to mitigate as they expect all calls to have a fee That said, given context, the gas optimization is valid, so we'll give the warden the points for it