Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

ERC1155's approval is not used on Silo transfers

T1MOH

Summary

Silo deposit is ERC1155 token, where address token || uint96 stem is tokenId. These deposits can be transferred via functions SiloFacet.transferDeposit() and SiloFacet.transferDeposits().

Additionally to ERC1155's allowance Silo has custom allowances to limit number of tokens. So there are 2 allowance mechanics:

struct Account {
...
mapping(address => mapping(address => uint256)) depositAllowances; //@note custom
...
mapping(address => bool) isApprovedForAll; //@note ERC1155
...
}

Problem is that ERC1155's allowance isApprovedForAll is never used on transfers, code uses only custom version.

Vulnerability Details

On transfers it spends allowance via LibSiloPermit._spendDepositAllowance():

function transferDeposit(
address sender,
address recipient,
address token,
int96 stem,
uint256 amount
) public payable fundsSafu noNetFlow noSupplyChange nonReentrant returns (uint256 _bdv) {
if (sender != LibTractor._user()) {
LibSiloPermit._spendDepositAllowance(sender, LibTractor._user(), token, amount);
}
...
}

Let's have a look on what it does. As you can see it doesn't matter if isApprovedForAll == true

function _spendDepositAllowance(
address owner,
address spender,
address token,
uint256 amount
) internal {
uint256 currentAllowance = depositAllowance(owner, spender, token);
if (currentAllowance != type(uint256).max) {
@> require(currentAllowance >= amount, "Silo: insufficient allowance");
_approveDeposit(owner, spender, token, currentAllowance - amount);
}
}
​
function depositAllowance(
address owner,
address spender,
address token
) public view returns (uint256) {
AppStorage storage s = LibAppStorage.diamondStorage();
@> return s.accts[owner].depositAllowances[spender][token];
}

Want to note that calling setApprovalForAll() doesn't affect another allowance:

function setApprovalForAll(
address spender,
bool approved
) external fundsSafu noNetFlow noSupplyChange {
@> s.accts[LibTractor._user()].isApprovedForAll[spender] = approved;
emit ApprovalForAll(LibTractor._user(), spender, approved);
}

Impact

ERC1155's allowance doesn't give permission to transfer deposits contrary to EIP1155.

Tools Used

Manual Review

Recommendations

Allow deposit transfers if operator is approved via EIP1155 allowance.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational/Gas

Invalid as per docs https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity

ERC1155's approval in Silo transfers

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.